fix(Drawer): expand only onTransitionEnd of the drawer #8801
Conversation
|
Preview: https://patternfly-react-pr-8801.surge.sh A11y report: https://patternfly-react-pr-8801-a11y.surge.sh |
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
This looks great! Just had a comment below that I'd appreciate your feedback on
| onTransitionEnd={(ev) => { | ||
| if (!hidden && ev.nativeEvent.propertyName === 'transform') { | ||
| onExpand(); | ||
| if ((ev.target as HTMLElement).id === (id || panelId)) { |
There was a problem hiding this comment.
An alternative to this:
| if ((ev.target as HTMLElement).id === (id || panelId)) { | |
| if ((ev.target as HTMLElement) === panel.current) { |
Not really sure how common an issue it'd be, but one thing I noticed when checking if the target id === the component id/panelId is that if we have something like the following:
<DrawerPanelContent id="tester">
Then if within the drawer if some element like an expandable toggle would be the target and have that same ID, it causes the original bug to resurface. In the clip below I just added id="tester" to .pf-c-expandable-section__toggle-icon via dev tools since I don't believe we could pass an ID to that exact element otherwise, but I'm more imagining a scenario where some custom expandable toggle is used instead.
Drawer.Panel.matching.ID.mov
Checking whether the target === the panel.current with the same setup as the above clip:
Drawer.Panel.matching.ref.mov
Personally I might lean more towards checking the target against the panel ref since ID duplication can happen, but like I said I'm not sure how common this would actually be. What do you think, @MariaAga ?
There was a problem hiding this comment.
Yeah I think its better to be safer, changed it
thatblindgeye
left a comment
thatblindgeye
left a comment
There was a problem hiding this comment.
🎉 Looks like there's just a snapshot that needs to be updated. @wise-king-sullyman was that issue with Timestamp tests resolved or is it still finicky?
|
rebased and now tests are green |
@thatblindgeye I disabled the timestamp test that was problematic. |
|
Your changes have been released in:
Thanks for your contribution! 🎉 |
4 participants
What: Closes #8510
can be tested by replacing
packages/react-core/src/components/Drawer/examples/DrawerStackedContentBodyElements.tsxwith: https://codesandbox.io/s/drawerstackedcontentbodyelements-t8v58c?file=/index.tsxand seeing that
onExpandcounter is not growing whenShow moreis clicked