Skip to content
This repository has been archived by the owner on Jan 5, 2023. It is now read-only.

feat: Add support of react 18 #656

Merged
merged 39 commits into from
May 27, 2022
Merged

feat: Add support of react 18 #656

merged 39 commits into from
May 27, 2022

Conversation

veekeys
Copy link
Member

@veekeys veekeys commented May 4, 2022

Updated to react 18.
Still need to do some testing (with Vite app). I still get warning in there, might be connected that the code has old render already, no matter if it is actually called... But on every call it is not shown using this solution, while without it, it shows on every toaster call.
Added kinda weird solution to toaster, just I think having it through setSettings might not work well, cause this is set globally and need to think of an option to reset it to undefined again.
Had this on pre commit hook, so went with --no-verify to commit
image

UPDATE:
As I was fighting different issues accidentally ended up upgrading few more libs (only jest and testing-library).

@veekeys veekeys self-assigned this May 4, 2022
@veekeys veekeys marked this pull request as ready for review May 6, 2022 10:23
@veekeys veekeys requested a review from a team as a code owner May 6, 2022 10:23
@veekeys veekeys requested review from a team, mayank99 and elephantcatdog and removed request for a team May 6, 2022 10:23
Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

I think PR title would need to be feat because this is adding react 18 support and also adding a new prop to Toaster.

Also, I think there is something wrong with TS still, because build is failing.

packages/iTwinUI-react/src/types/toaster.ts Outdated Show resolved Hide resolved
packages/iTwinUI-react/src/types/custom.d.ts Outdated Show resolved Hide resolved
packages/iTwinUI-react/src/core/Toast/Toaster.tsx Outdated Show resolved Hide resolved
packages/iTwinUI-react/src/core/Toast/Toaster.tsx Outdated Show resolved Hide resolved
packages/iTwinUI-react/stories/core/Toasts.stories.tsx Outdated Show resolved Hide resolved
"jest-junit": "^12.0.0",
"jest": "^28.1.0",
"jest-cli": "^28.1.0",
"jest-environment-jsdom": "^28.1.0",
Copy link
Member Author

Choose a reason for hiding this comment

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

expect(rows[1].classList).not.toContain('iui-selected');
expect(rows[2].classList).toContain('iui-selected');
expect(onSelect).toHaveBeenCalledTimes(2);
expect(onRowClick).toHaveBeenCalledTimes(2);

userEvent.click(getByText(mockedData()[1].name), { ctrlKey: true });
const user = userEvent.setup();
Copy link
Member Author

Choose a reason for hiding this comment

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

const mockContainer = mockDocument.querySelector('div') as HTMLDivElement;

it('should render filter dropdown in the correct document', async () => {
const mockDocument = document.implementation.createHTMLDocument();
Copy link
Member Author

Choose a reason for hiding this comment

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

It was failing for me before... This helped. I have verified it by getting rid of ownerDocument in filter and this test started to fail.

@@ -234,15 +234,15 @@ describe('<Wizard />', () => {

expect(document.querySelector('.iui-tooltip')).toBeNull();
expect(screen.queryByText('Step one tooltip')).toBeNull();
userEvent.hover(screen.getByText('Step One'));
fireEvent.mouseEnter(screen.getByText('Step One'), { bubbles: true });
Copy link
Member Author

Choose a reason for hiding this comment

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

We hover over span, but only parent li has the popover to trigger tooltip.
userEvent didnt have (or I didnt find) bubbling option, so used fireEvent here instead.

@veekeys
Copy link
Member Author

veekeys commented May 20, 2022

There is this Carousel controlled story, which is failing... It does not keep the manually set slide. I might need @mayank99 insights on certain part of the code..

I spent a few hours trying to play around with different permutations of intersection observers, scroll events, effects, callbacks, etc, but could not get all three cases to work with the same code:

  • uncontrolled with manual slide change (e.g. clicking next/prev or pressing ←/→ )
  • controlled
  • automatic slide change (e.g. dragging on mobile, or focusing an element inside a slide)

Update: I was able to fix the Carousel after playing around some more. It works with all three cases now.

Not sure if we want to make it part of this PR so I pushed it to a separate branch: bd7c8a8

Would you mind creating a separate PR? I wanted to, but I saw you have all react 18 changes in there..
Would be nice to merge that before react 18, just to be sure.

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

Update: I was able to fix the Carousel after playing around some more. It works with all three cases now.
Not sure if we want to make it part of this PR so I pushed it to a separate branch: bd7c8a8

Would you mind creating a separate PR? I wanted to, but I saw you have all react 18 changes in there.. Would be nice to merge that before react 18, just to be sure.

Created #683.

packages/iTwinUI-react/src/core/Toast/Toaster.tsx Outdated Show resolved Hide resolved
packages/iTwinUI-react/stories/core/Toasts.stories.tsx Outdated Show resolved Hide resolved
@veekeys
Copy link
Member Author

veekeys commented May 23, 2022

I'm getting this error in browser console when I open storybook.

The pseudo class ":first-child" is potentially unsafe when doing server-side rendering. Try changing it to ":first-of-type"

It doesn't really affect us because we are not server rendering storybook, but it is polluting the entire browser console. Any way to suppress it? image

Yeah, I was checking it and somehow tohught prod build should not have it, but forgot to test.. I will see, if I find anything.. I think I saw somewhere, that this might be coming from storybook itself (it dedected first-child selector in its own css)

Issue to follow: storybookjs/storybook#18103

Copy link
Contributor

@mayank99 mayank99 left a comment

Choose a reason for hiding this comment

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

Looks good 👍

@veekeys veekeys changed the title feat: Update to react 18 feat: Add support of react 18 May 27, 2022
@veekeys veekeys merged commit b7a4c7b into main May 27, 2022
@veekeys veekeys deleted the vyki/react-18-upgrade branch May 27, 2022 08:38
@mayank99 mayank99 linked an issue May 27, 2022 that may be closed by this pull request
4 tasks
mayank99 pushed a commit to iTwin/iTwinUI that referenced this pull request Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

React 18 support
4 participants