Skip to content

Conversation

@Infinite-Null
Copy link
Contributor

@Infinite-Null Infinite-Null commented Dec 13, 2024

Part of: #67813

What?

Refactored Avatar Block code to include ToolsPanel instead of PanelBody.

Screenshots:

Before After
image image

@Infinite-Null Infinite-Null marked this pull request as ready for review December 16, 2024 05:40
@github-actions
Copy link

github-actions bot commented Dec 16, 2024

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: Infinite-Null <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: fabiankaegy <[email protected]>

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

Copy link
Contributor

@t-hamano t-hamano left a 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):

avatar-panel-order

We'll probably need to resolve that issue before we can ship this PR.

@Mamaduka
Copy link
Member

Hi, @Infinite-Null

Do you have time to follow up on this PR?

@Infinite-Null
Copy link
Contributor Author

Sure @Mamaduka

@Infinite-Null Infinite-Null force-pushed the refactor/avatar-block-settings-panel branch from 9b18cd9 to 15984cc Compare May 30, 2025 15:34
@Infinite-Null Infinite-Null requested a review from t-hamano May 30, 2025 15:58
@Infinite-Null
Copy link
Contributor Author

Hi @Mamaduka and @t-hamano ,
I’ve made the necessary changes and moved the conditional addition of settings to the very end so that it matches the expected order. Please let me know if there’s anything else I should update!

image

@t-hamano
Copy link
Contributor

moved the conditional addition of settings to the very end so that it matches the expected order.

I don't think this is an ideal solution. This may be a problem that should be fixed on the ToolsPanel component side.

For now, I personally prefer to accept this issue and prioritize the migration to the TooslPanel component. See #67813 (comment)

@Mamaduka @fabiankaegy What do you think?

@Infinite-Null
Copy link
Contributor Author

Sure @t-hamano, I have reverted my refactor changes 😄

Copy link
Contributor

@t-hamano t-hamano left a 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.

@Infinite-Null
Copy link
Contributor Author

Sure @t-hamano 🎉

@Mamaduka
Copy link
Member

For now, I personally prefer to accept this issue and prioritize the migration to the TooslPanel component

Makes sense. Retaining correct ordering is a general SlotFill issue.

@Mamaduka
Copy link
Member

A small interesting observation regarding ComboboxControl. I noticed that resetting the userId value to undefined doesn't clear out the combobox selection. It works if I change the reset value to null.

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

Screencast

CleanShot.2025-06-12.at.16.16.13.mp4

@Mamaduka Mamaduka merged commit 8586e63 into WordPress:trunk Jun 12, 2025
60 checks passed
@github-actions github-actions bot added this to the Gutenberg 21.1 milestone Jun 12, 2025
@ciampo
Copy link
Contributor

ciampo commented Jun 24, 2025

@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 null (and not undefined).

@t-hamano
Copy link
Contributor

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 null (and not undefined).

After looking into this, it seems like this is indeed intentional behavior.

The ComboboxControl component uses the useControlledValue hook internally. In the useControlledValue hook, if the value is undefined, it is considered to be in an "uncontrolled state":

const hasValue = typeof valueProp !== 'undefined';

Related PRs:

@Mamaduka
Copy link
Member

That makes sense, thank you both for the feedback.

chriszarate pushed a commit to chriszarate/gutenberg that referenced this pull request Jul 1, 2025
Co-authored-by: Infinite-Null <[email protected]>
Co-authored-by: t-hamano <[email protected]>
Co-authored-by: Mamaduka <[email protected]>
Co-authored-by: fabiankaegy <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Avatar Affects the Avatar Block [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor "Settings" panel of Avatar block to use ToolsPanel instead of PanelBody

5 participants