Skip to content

updated * to itwinui 3 #141

Closed
nimam-bentley wants to merge 19 commits into
mainfrom
nm/update-imodelbrowser-to-itwinui3
Closed

updated * to itwinui 3 #141
nimam-bentley wants to merge 19 commits into
mainfrom
nm/update-imodelbrowser-to-itwinui3

Conversation

@nimam-bentley
Copy link
Copy Markdown
Contributor

@nimam-bentley nimam-bentley commented Jan 16, 2025

  • update itwinui versions in imodel-browser to latest 3.x
  • upgraded legacy props to components, including adding Tile.Action area
  • improves accessibility

UPDATE

  • updated all remaining packages to latest itwinui 3.x
  • ^ this broke a bunch of tests, most of the changes in this pr are now for fixing them
  • ^ part of this is updating @testing-library and adjusting some of the methods/locators

Note

would make some suggestions, pending state at merge, separate issues can be created

  • migrate from .iui- classes to itwinui components
  • use preferable locators [partially done in this pr]
  • shift test assertions of implementation details to user expectations, ensuring accessibility

@nimam-bentley nimam-bentley self-assigned this Jan 16, 2025
"@itwin/itwinui-icons-react": "^2.2.0",
"@itwin/itwinui-react": "^2.12.18",
"@itwin/itwinui-icons-react": "^2.9.0",
"@itwin/itwinui-react": "^3.16.5",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think itiwnui would need to become a peer dep

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated all packages in monorepo

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont see this as a dev+peer dep

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also use the latest version, 3.17, to add support for the theme bridge

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont see this as a dev+peer dep

its used in prod implementation in this package. i could add it as a peer but i dont think anything else here would have the same subdep on itwinui-react? why in dev if needed in prod?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also use the latest version, 3.17, to add support for the theme bridge

updated to latest 3.17.3

Comment thread packages/modules/imodel-browser/pnpm-lock.yaml Outdated
Comment thread packages/modules/imodel-browser/src/containers/ITwinGrid/ITwinTile.tsx Outdated
@nimam-bentley nimam-bentley changed the title updated imodel-browser to itwinui 3 updated * to itwinui 3 Jan 24, 2025
@aruniverse aruniverse mentioned this pull request Jan 29, 2025
@DanishMehmood-bit
Copy link
Copy Markdown
Contributor

DanishMehmood-bit commented Mar 16, 2025

Can we bump the priority on this one, since iTwin for Desktop wants to upgrade to itwinui's theme bridge and according to their wiki all of the dependencies should be on v3 and we depend on imodel-browser-react pkg
image

@aruniverse @nimam-bentley @kckst8

Copy link
Copy Markdown
Member

@aruniverse aruniverse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

plz review existing comments?
is there a need to update every single pkg in this monorepo for your app?

"@itwin/itwinui-icons-react": "^2.2.0",
"@itwin/itwinui-react": "^2.12.18",
"@itwin/itwinui-icons-react": "^2.9.0",
"@itwin/itwinui-react": "^3.16.5",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i dont see this as a dev+peer dep

"tslib": "^2.6.2",
"typescript": "^4.2.3"
"typescript": "^4.2.3",
"@testing-library/dom": "~10.4.0",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets alphabetize our deps

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lets alphabetize our deps

moved the '@' prefixed deps

Comment thread packages/apps/storybook/package.json Outdated
@@ -13,7 +13,7 @@
"@itwin/delete-imodel-react": "^2.0.0",
"@itwin/delete-itwin-react": "^2.0.0",
"@itwin/imodel-browser-react": "~2.3.1",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt these (pkgs within the monorepo) be using the workspace:* protocol?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldnt these (pkgs within the monorepo) be using the workspace:* protocol?

that would make sense, however workspace does not appear to be configured in this repo perhaps that change should be in its own pr

"@itwin/itwinui-icons-react": "^2.2.0",
"@itwin/itwinui-react": "^2.12.18",
"@itwin/itwinui-icons-react": "^2.9.0",
"@itwin/itwinui-react": "^3.16.5",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should also use the latest version, 3.17, to add support for the theme bridge

@@ -11,28 +11,24 @@ import React, { forwardRef } from "react";
export const IModelGhostTile = forwardRef<HTMLDivElement>((props, ref) => {
return (
<ThemeProvider ref={ref} theme="inherit" {...props}>
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the plan is to use the theme bridge, then wouldnt you need to expose that for every single component that you wrap with theme provider?

would it make more sense to not wrap anything in theme provider, and expect the host/parent app to do so instead?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if the plan is to use the theme bridge, then wouldnt you need to expose that for every single component that you wrap with theme provider?

would it make more sense to not wrap anything in theme provider, and expect the host/parent app to do so instead?

i think it would be fine given the theme is set to inherit?
https://itwinui.bentley.com/docs/themeprovider#inheritance

@nimam-bentley
Copy link
Copy Markdown
Contributor Author

plz review existing comments? is there a need to update every single pkg in this monorepo for your app?

i only needed imodel-browser updated. its been some time but i recall having some issues updating only that package, and figured they would all need to be updated eventually anyway so might aswell. i think updating the whole repo is also the goal of work item

@aruniverse
Copy link
Copy Markdown
Member

closing in favor of #144

@aruniverse aruniverse closed this Apr 1, 2025
@mayank99
Copy link
Copy Markdown
Collaborator

mayank99 commented Apr 1, 2025

closing in favor of #144

@aruniverse #144 is only concerned with updating @itwin/imodel-browser-react. What about all other packages in this monorepo? Are we abandoning those packages?

@aruniverse
Copy link
Copy Markdown
Member

closing in favor of #144

@aruniverse #144 is only concerned with updating @itwin/imodel-browser-react. What about all other packages in this monorepo? Are we abandoning those packages?

I wouldn't say abandon, but if we have no consumers asking for iTwinUI v3 versions of it, then we can defer it

@simihartstein
Copy link
Copy Markdown
Contributor

OpenSite+ is not using any of the other packages in this repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants