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

fix: [useCollapse] cleanup resize observer, overflow css #1577

Merged
merged 1 commit into from
Mar 10, 2023

Conversation

renrizzolo
Copy link
Contributor

@renrizzolo renrizzolo commented Mar 8, 2023

Description

  • as per ResizeObserver#observe() firing the callback immediately results in infinite loop WICG/resize-observer#38, wrap the observer in requestAnimationFrame so we don't get the Loop limit exceeded error.
  • Fix panel overflow/dispaly styles by adding transition state to useCollapse, so we can hide overflow only when animating
    • previously I set hidden overflow on the panel so you wouldn't see the content outside of the panel when toggling the collapse. Discovered we have some useage of Panel with Select inside them, which would be hidden by the overflow.
  • some cleanup to the collapse internals

Does this PR introduce a breaking change?

  • Yes
  • No

Manual testing step?

Screenshots (if appropriate):

no overflow / without fix
Kapture 2023-03-09 at 11 29 13

with overflow/display transition classes
Kapture 2023-03-09 at 11 28 28

@renrizzolo renrizzolo force-pushed the fix-resize-observer-error branch 4 times, most recently from 8edfac7 to fdc9bc3 Compare March 9, 2023 00:09
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #1577 (38345fd) into master (70db3fd) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master     #1577   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           96        96           
  Lines         1695      1706   +11     
  Branches       471       478    +7     
=========================================
+ Hits          1695      1706   +11     
Impacted Files Coverage Δ
src/components/Panel/index.jsx 100.00% <100.00%> (ø)
src/components/Paragraph/index.jsx 100.00% <100.00%> (ø)
src/hooks/useCollapse.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

shn2016
shn2016 previously approved these changes Mar 9, 2023
romi-h
romi-h previously approved these changes Mar 9, 2023
Comment on lines 53 to 55
React.useEffect(() => {
setCollapsed(collapsedProp);
}, [collapsedProp]);
Copy link
Contributor

Choose a reason for hiding this comment

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

guess this is because both collapsedProp and toggleCollapsed are trying control collapsed state
i think should be either
(defaultCollapsed, ...) => {collapsed, toggleCollapsed, ...}
or
(colleapsed, ...) => {...}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, defaultCollapsed is better

Copy link
Contributor

Choose a reason for hiding this comment

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

then i guess this can be removed as default props shouldn't be affecting the element state during its life cycle, the same as defaultValue to input component

React.useEffect(() => {
setCollapsed(defaultCollapsed);
}, [defaultCollapsed]);

Copy link
Contributor Author

@renrizzolo renrizzolo Mar 9, 2023

Choose a reason for hiding this comment

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

oh, it's still being used to control the component, so I think I'll revert the naming to what it was before

Copy link
Contributor

Choose a reason for hiding this comment

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

what i mean was both toggleCollapsed and collapsedProp control the collapse state, should be just keeping one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see what you mean, I'll remove the toggleCollapsed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@knilink removed internal collapsed state and toggle function

src/hooks/useCollapse.js Outdated Show resolved Hide resolved
@renrizzolo renrizzolo dismissed stale reviews from romi-h and shn2016 via 45a300f March 9, 2023 04:11
@renrizzolo renrizzolo force-pushed the fix-resize-observer-error branch 5 times, most recently from 91da30c to 7ad4973 Compare March 10, 2023 01:48
@sonarcloud
Copy link

sonarcloud bot commented Mar 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 1 Code Smell

No Coverage information No Coverage information
0.0% 0.0% Duplication

@xiaofan2406 xiaofan2406 merged commit 115b990 into master Mar 10, 2023
@xiaofan2406 xiaofan2406 deleted the fix-resize-observer-error branch March 10, 2023 03:29
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.

5 participants