CARVIEW |
Navigation Menu
-
Notifications
You must be signed in to change notification settings - Fork 4.6k
Avatar: Refactor settings panel to use ToolsPanel #67952
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
Avatar: Refactor settings panel to use ToolsPanel #67952
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. |
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.
Thanks for the PR!
Everything is working as expected, but there are some reported issues with the order of the controls not matching the order of the dropdown items (See this comment):
We'll probably need to resolve that issue before we can ship this PR.
Hi, @Infinite-Null Do you have time to follow up on this PR? |
Sure @Mamaduka |
9b18cd9
to
15984cc
Compare
I don't think this is an ideal solution. This may be a problem that should be fixed on the For now, I personally prefer to accept this issue and prioritize the migration to the @Mamaduka @fabiankaegy What do you think? |
Sure @t-hamano, I have reverted my refactor changes 😄 |
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! Let's wait and see what others think about this comment before merging.
Sure @t-hamano 🎉 |
Makes sense. Retaining correct ordering is a general SlotFill issue. |
A small interesting observation regarding This could be an intentional side-effect or a bug. I think it's worth investigating separately, if we can reproduce the same problem in isolation. cc @WordPress/gutenberg-components ScreencastCleanShot.2025-06-12.at.16.16.13.mp4 |
@Mamaduka I'd have to look deeper into the code to let you know whether this is working as intended or a bug — but, at least philosophically, it does follow the controlled/uncontrolled pattern that we've implementing rencently on some component: when a component is controlled, "no value" should be expressed with |
After looking into this, it seems like this is indeed intentional behavior. The
Related PRs: |
That makes sense, thank you both for the feedback. |
Co-authored-by: Infinite-Null <ankitkumarshah@git.wordpress.org> Co-authored-by: t-hamano <wildworks@git.wordpress.org> Co-authored-by: Mamaduka <mamaduka@git.wordpress.org> Co-authored-by: fabiankaegy <fabiankaegy@git.wordpress.org>
Part of: #67813
What?
Refactored Avatar Block code to include ToolsPanel instead of PanelBody.
Screenshots: