Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

replace useTheme with internal useGlobals #1265

Merged
merged 22 commits into from May 17, 2023
Merged

Conversation

mayank99
Copy link
Contributor

@mayank99 mayank99 commented May 9, 2023

Changes

  • Removed useTheme hook, which means ThemeProvider is now required.
  • ThemeProvider must always be used with children.
  • Removed the deprecated ownerDocument option in ThemeProvider.
  • Added new useGlobals hook in every component. Will be used for global setup (e.g. context for Future: Scoped sizing using Context #969 and useToaster) and side-effects (e.g. css imports and console warnings).

Testing

Everything seems to work same as before when app is wrapped in <ThemeProvider>.

When not wrapped, the warning is correctly displayed in dev environment.

Docs

Changeset added.

Migration guide updated: https://github.com/iTwin/iTwinUI/wiki/iTwinUI-react-v3-migration-guide#breaking-changes

@mayank99 mayank99 self-assigned this May 9, 2023
@mayank99 mayank99 mentioned this pull request May 9, 2023
22 tasks
@mayank99 mayank99 changed the title remove useTheme hook replace useTheme with internal useGlobals May 9, 2023
@mayank99 mayank99 added this to the React 3.0 milestone May 9, 2023
@mayank99

This comment was marked as outdated.

@mayank99

This comment was marked as outdated.

@mayank99 mayank99 marked this pull request as ready for review May 10, 2023 23:19
@mayank99 mayank99 requested a review from a team as a code owner May 10, 2023 23:19
@mayank99 mayank99 requested review from a team, gretanausedaite and LostABike and removed request for a team May 10, 2023 23:19
@gretanausedaite
Copy link
Contributor

gretanausedaite commented May 12, 2023

@gretanausedaite Maybe you want to investigate?

Controlled tooltip is also broken :)

It appears that Popover does not get rootRef from ThemeProvider therefore adds content to owerDocument.body.
I added id='iui-root' in theme provider for testing purposes:
image

And added a query for that in Popover:
image

And now Select with controlled Popover and Controlled Tooltip works:
image

My guess this is the same reason why in some cases Popover does not append itself to our theme provider root. (eg. recent ColorPicker issue, AppUI select issue)

Maybe we can add data attribute to ThemeProvider for Popover to look for?

@mayank99
Copy link
Contributor Author

mayank99 commented May 12, 2023

Controlled tooltip is also broken

Both fixed. It looks like rootRef.current was undefined on first re-render and correctly set on second render, so when using visible: true, it was appending to body.

Fixed by using key. See d3c5c9e

Edit: key was causing other issues, so I'm now using a check for useIsDomAvailable. Everything seems to work and all tests pass. See c92565f.

When we implement our custom Popover using floating-ui, we will need to do something similar to wait until rootRef is available.

@mayank99 mayank99 changed the title replace useTheme with internal useGlobals v3: replace useTheme with internal useGlobals May 12, 2023
Copy link
Contributor

@LostABike LostABike left a comment

Choose a reason for hiding this comment

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

Ok I have tried to test and toggle globals for everything that has more than a name change, and some random ones with only name change and everything seems to work fine for me (Windows + Chrome). Have also read the actual code changes.

Copy link
Contributor

@gretanausedaite gretanausedaite left a comment

Choose a reason for hiding this comment

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

Toasts get theme warning in Storybook. Probably because it's storybook.

LGTM.

@mayank99
Copy link
Contributor Author

Toasts get theme warning in Storybook. Probably because it's storybook.

I think it's expected, because we are calling a separate createRoot inside Toaster. Should be fixed when we provide new useToaster api.

@mayank99 mayank99 changed the base branch from main to dev May 17, 2023 19:04
@mayank99 mayank99 changed the title v3: replace useTheme with internal useGlobals replace useTheme with internal useGlobals May 17, 2023
@mayank99 mayank99 merged commit a7e7536 into dev May 17, 2023
12 checks passed
@mayank99 mayank99 deleted the mayank/remove-usetheme branch May 17, 2023 19:27
@imodeljs-admin imodeljs-admin mentioned this pull request Oct 23, 2023
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.

None yet

3 participants