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.

Sign up for GitHub

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

Jump to bottom

Since switching to createRoute (concurrent React), inline styles are sometimes not updated #103

Closed
adrienharnay opened this issue Oct 24, 2022 · 15 comments · Fixed by #122, #125 or #142
Closed

Since switching to createRoute (concurrent React), inline styles are sometimes not updated #103

adrienharnay opened this issue Oct 24, 2022 · 15 comments · Fixed by #122, #125 or #142
Labels
released on @next

Comments

@adrienharnay
Copy link
Contributor

Hello,

Since switching to React 18 and react-dom/client's createRoute, when I try to open the Collapse (passing isExpanded: true to getCollapseProps) after the first render the inline styles don't get updated: https://gyazo.com/9c552baad3819a12908c12a85288e223. I need to set isExpanded to false, and then to true for the inline styles to be correct.

I don't have this issue with React.render (non-concurrent React API).

Thanks in advance and have a great day!
@adrienharnay
Copy link
Contributor Author

adrienharnay commented Oct 27, 2022 •
edited
Loading

Reverting to 3.3.2 (without #98) fixes the issue. Any idea how upgrading and using React's useId could generate this bug? cc. @gaearon @roginfarrer

I was wrong. 3.3.2 has the bug as well. The bug is only related to the usage of createRoot instead of render, and only when you don't mount children when isExpanded === false.

Here is a Sandbox reproducing the bug: https://codesandbox.io/s/sweet-mountain-u7docf. You can change the import:

import { CollapseWithoutBug as Collapse } from "./Collapse";

to

import { CollapseWithBug as Collapse } from "./Collapse";

to trigger the bug.

You can also change the rendering method (switch back to render) to notice both versions are working.

To sum it up:

  • using react-dom's render, the bug never occurs
  • using react-dom/client's createRoot, the bug does not occur if children are always mounted
  • using react-dom/client's createRoot, if children are unmounted (see CollapseWithBug), the bug occurs

Maybe related to #72 ?

@adrienharnay
Copy link
Contributor Author

OK so I tested again after adding logs everywhere to try and understand the root cause. It would seem that when the bug occurs, the onTransitionEnd handler passed to the child element is not called by React... 🫠 Is it a side-effect or the root cause?

@roginfarrer roginfarrer mentioned this issue Nov 26, 2022
Merged
roginfarrer added a commit that referenced this issue Nov 27, 2022
@roginfarrer
feat: refactor to vanilla JS core (#119)
4de85da 
## BREAKING CHANGES

- Adopts React 18's `useId`, making the library incompatible with React <18
- Switch to `tsup` from `microbundle` for bundling library. No longer exports a UMD version, just CJS and MJS

## Features & Bug fixes

- Refactors core functionality to vanilla JS (with a React) adapter, which I think should fix #103 and fix #100 
- Added `onExpandedChange` option
- Tries to detect if `getToggleProps` is used. If the toggle element ref can be accessed, the `aria-labelledby` attribute will be added to the collapse element
- Added `role="region"` to collapse
- Updated toggle props to pass the appropriate attributes to the element, whether it's a button or not
@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 3.7.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

roginfarrer pushed a commit that referenced this issue Nov 27, 2022
feat: refactor to vanilla JS core
a022515 
BREAKING CHANGES

- Adopts React 18's `useId`, making the library incompatible with React <18
- Switch to `tsup` from `microbundle` for bundling library. No longer exports a UMD version, just CJS and MJS

Features & Bug fixes

- Refactors core functionality to vanilla JS (with a React) adapter, which I think should fix #103 and fix #100
- Added `onExpandedChange` option
- Tries to detect if `getToggleProps` is used. If the toggle element ref can be accessed, the `aria-labelledby` attribute will be added to the collapse element
- Added `role="region"` to collapse
- Updated toggle props to pass the appropriate attributes to the element, whether it's a button or not
@github-actions
Copy link
Contributor

🎉 This issue has been resolved in version 3.8.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@roginfarrer
Copy link
Owner

Meant to release at v4@next, but couldn't figure out the magic words for semantic-release 🤷🏻. Can you try testing with react-collapsed@next @adrienharnay?

@roginfarrer roginfarrer mentioned this issue Dec 4, 2022
Merged
@adrienharnay
Copy link
Contributor Author

Hello, sorry for the delay. I just tested and got the useId is not a function error

Screenshot 2022-12-12 at 08 39 04

Versions:

Screenshot 2022-12-12 at 08 39 42

I have also tested with 4.0.0, same result.

@adrienharnay
Copy link
Contributor Author

Hello, have you had a chance to look at the error? Thanks in advance @roginfarrer

@roginfarrer
Copy link
Owner

I've briefly looked at it and I can replicate, but I'm not 100% sure why that's happening. I have a major refactor ongoing in #122, and I'm hoping this issue resolves itself

@nmanu1 nmanu1 mentioned this issue Jan 4, 2023
Merged
nmanu1 added a commit to yext/search-ui-react that referenced this issue Jan 4, 2023
@nmanu1
Pin react-collapsed to minor version (#350)
cb546c5 
This PR pins `react-collapsed` to the latest minor version (v3.6.0) that supports React 17. In v3.7.0 and v3.8.0, `react-collapsed` fixed a bug ([Slack thread](https://yext.slack.com/archives/C032CKFARGS/p1671130163844249), [GH issue](roginfarrer/collapsed#103)) that occurs when using it with React 18 by introducing a breaking change where they dropped compatibility with React 16 and 17. v3.7.0+ would break a search experience using collapsible facets with React <18. Since they are not able to support both React 17 and 18 together, it seems preferable to pin to a version that can be used with both, but is a little buggy with React 18, rather than fully functional with React 18, but breaks other React experiences.

J=SLAP-2521
TEST=manual

See that collapsible facets works as expected on a test-site running on React 17 and that the site running on React 18 still has most functionality, except for the bug described above.
@roginfarrer
Copy link
Owner

I'm not sure about the React version error, but I was able to reimplement this sandbox with Vite, and everything looked okay with me. Maybe something to do with CRA?

@github-actions github-actions bot mentioned this issue Jan 16, 2023
Merged
@adrienharnay
Copy link
Contributor Author

4.0.0 does not seem to be released on npm

@roginfarrer
Copy link
Owner

The library was refactored and is now published under @collapsed/react https://github.com/roginfarrer/collapsed/releases/tag/%40collapsed%2Freact%404.0.0

@adrienharnay
Copy link
Contributor Author

Hello, this is the link to a GitHub release but I can't seem to find the library on npm?

@roginfarrer
Copy link
Owner

NPM

heads up that I'm currently working through some bugs from the refactor right now, so it's not 100% ready. It may work for you, though.

@roginfarrer roginfarrer reopened this Mar 2, 2023
@roginfarrer
Copy link
Owner

@adrienharnay can you try this version: @collapsed/react@4.1.0-experimental.79f518c

@github-actions github-actions bot mentioned this issue Apr 6, 2023
Merged
@adrienharnay
Copy link
Contributor Author

Hi, I tried the version and everything is going well. When are you planning on deprecating the old package and switching to the new one?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
released on @next
Projects
None yet
Milestone
No milestone
2 par