-
Notifications
You must be signed in to change notification settings - Fork 861
[Flyout System] Ensure the push padding for managed flyout does not remain for other session #9207
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
[Flyout System] Ensure the push padding for managed flyout does not remain for other session #9207
Conversation
| * before child components render. This prevents RemoveScrollBar from measuring the body | ||
| * in an inconsistent state during flyout transitions. | ||
| */ | ||
| useLayoutEffect(() => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving the padding logic into useLayoutEffect ensures the body is updated before paint and before any measurement by scroll-lock utilities
| const managerSide = side === 'left' ? 'left' : 'right'; | ||
|
|
||
| // EUI doesn't use this css variable, but it is useful for consumers | ||
| if (shouldApplyPadding) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code inside the if and else handle where padding should be set or cleared based on a push flyout being active in its session
|
/ci |
|
Changes look good and work as expected. I see that the branch history is a bit broken - @tsullivan would you be able to discard the old merge commits and rebase/merge the latest |
41f8990 to
e2b53f0
Compare
…dding only for active flyouts and disabling scroll lock for overlay flyouts when push padding exists.
416f2b2 to
1b725ae
Compare
💚 Build SucceededHistory
|
💚 Build Succeeded
History
|
tkajtoch
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 🚢
Summary
This fix makes push padding conditional for managed push flyouts: padding is only applied when the flyout is active in its session. When a flyout becomes active, it updates push padding data in the Flyout Manager's state. When a flyout becomes inactive, it explicitly clears the padding.
Unmanaged push flyouts continue to behave as before, always applying padding.
Why are we making this change?
Closes #9159
Screenshots #
Before
After
Impact to users
Higher quality UX when multiple flyout sessions are used and they are configured differently.
QA
Remove or strikethrough items that do not apply to your PR.
General checklist
[ ] Checked in both MacOS and Windows high contrast modes(emulate forced colors if you do not have access to a Windows machine.)[ ] Added documentation[ ] Props have proper autodocs (using@defaultif default values are missing) and playground toggles[ ] Checked Code Sandbox works for any docs examples[ ] Updated visual regression tests[ ] A changelog entry exists and is marked appropriately[ ] If applicable, added the breaking change issue label (and filled out the breaking change checklist)[ ] If the changes unblock an issue in a different repo, smoke tested carefully (see Testing EUI features in Kibana ahead of time)[ ] If applicable, file an issue to update EUI's Figma library with any corresponding UI changes. (This is an internal repo, if you are external to Elastic, ask a maintainer to submit this request)