Repository navigation
ToggleGroupControl: clean up animation logic - #65808
Conversation
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
DaniGuardiola
added
[Type] Code Quality
ciampo
left a comment
ciampo
left a comment
There was a problem hiding this comment.
Thank you for working on this!
Changes are testing well in Storybook and the editor.
Left a couple of comments about the API design of the hook, but all pretty small stuff. Snapshots need updating too.
Next steps are to move useAnimatedOffsetRect to the shared utils folder and re-use it in Tabs too, right?
Side note: it would be a nice touch to add an animation to the indicator when it's shown/hidden, even a simple fade. I believe that's also how it worked with the previous implementation. Could we do that in a follow-up?
|
Yes, follow up is to move to utils and use in Tabs. About the fade, I'm not sure I like it or see it necessary. Maybe worth proposing in an issue and looping design folks in. I personally like things to be snappy and a fade goes against that. |
👌
Separate issue + design folks ping SGTM 👍 |
|
Addressed your feedback @ciampo. Thanks! Edit: also updated snapshots. |
What?
Misc. cleanups and improvements of the
useSubelementAnimation(now renamed touseAnimatedOffsetRect).Why?
To improve clarity and also to later hoist and reuse the same utility in
Tabs.Tablist.How?
Functionally, the implementation is unchanged. However, I made the following improvements:
TabList, where the rect is used for other purposes.Testing Instructions
Check that the ToggleGroupControl animation still works as expected, both in Storybook and in Gutenberg instances.
Demo
chrome_a72hnAGpiY.mp4