Skip to content

ToggleGroupControl: clean up animation logic #65808

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
Merged
merged 4 commits into from
Oct 3, 2024
Merged

ToggleGroupControl: clean up animation logic #65808

merged 4 commits into from
Oct 3, 2024

Conversation

DaniGuardiola
Copy link
Member

@DaniGuardiola DaniGuardiola commented Oct 1, 2024 •
edited
Loading

What?

Misc. cleanups and improvements of the useSubelementAnimation (now renamed to useAnimatedOffsetRect).

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:

  • Various renames.
  • API now takes the rect in instead of creating it. This will enable its later use in TabList, where the rect is used for other purposes.
  • Switched from CSS class to data attribute. Class was fine, but this way it should be a bit more resilient to potential React reconciliation conflicts.
  • Improved docs with more details and better clarity.
  • Parent (now "container") extracted from options and made mandatory (though nullable) so that users can't miss it (I made this mistake myself recently and it caused a bug). It doesn't default to the overflow parent of the subelement anymore either.

Testing Instructions

Check that the ToggleGroupControl animation still works as expected, both in Storybook and in Gutenberg instances.

Demo

chrome_a72hnAGpiY.mp4

@DaniGuardiola DaniGuardiola self-assigned this Oct 1, 2024
@github-actions GitHub Actions
Copy link

github-actions bot commented Oct 1, 2024 •
edited
Loading

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 props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: DaniGuardiola <daniguardiola@git.wordpress.org>
Co-authored-by: ciampo <mciampini@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@DaniGuardiola DaniGuardiola added [Type] Code Quality Issues or PRs that relate to code quality [Package] Components /packages/components labels Oct 1, 2024
@DaniGuardiola DaniGuardiola requested a review from a team October 1, 2024 21:18
ciampo
ciampo reviewed Oct 2, 2024
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

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?

@DaniGuardiola
Copy link
Member Author

DaniGuardiola commented Oct 2, 2024 •
edited
Loading

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.

@ciampo
Copy link
Contributor

ciampo commented Oct 2, 2024

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 👍

@DaniGuardiola
Copy link
Member Author

DaniGuardiola commented Oct 2, 2024 •
edited
Loading

Addressed your feedback @ciampo. Thanks!

Edit: also updated snapshots.

@DaniGuardiola DaniGuardiola requested a review from ciampo October 2, 2024 18:07
ciampo
ciampo approved these changes Oct 2, 2024
View reviewed changes
Copy link
Contributor

@ciampo ciampo left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

@DaniGuardiola DaniGuardiola merged commit 8e3146f into trunk Oct 3, 2024
63 of 64 checks passed
@DaniGuardiola DaniGuardiola deleted the improve/clean-up-animation-utility branch October 3, 2024 00:53
@github-actions github-actions bot added this to the Gutenberg 19.4 milestone Oct 3, 2024
@DaniGuardiola DaniGuardiola mentioned this pull request Oct 4, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ciampo ciampo ciampo approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra ajitbohra is a code owner

Assignees

@DaniGuardiola DaniGuardiola

Labels
[Package] Components /packages/components [Type] Code Quality Issues or PRs that relate to code quality
Projects
None yet
Milestone
Gutenberg 19.4
Development

Successfully merging this pull request may close these issues.
2 participants
@DaniGuardiola @ciampo