Skip to content
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

Fix ToggleGroupControlOption not passing ref to the underlying element #35546

Merged
merged 3 commits into from
Oct 13, 2021
Merged

Fix ToggleGroupControlOption not passing ref to the underlying element #35546

merged 3 commits into from
Oct 13, 2021

Conversation

diegohaz
Copy link
Member

This PR is a small fix to avoid future issues.

The internal ToggleGroupControlButton component doesn't accept a ref prop. Instead, it expects to receive forwardedRef:

gutenberg/packages/components/src/toggle-group-control/toggle-group-control-button.tsx

Line 23 in 17bb52b

forwardedRef,

Because of this, passing ref to ToggleGroupControlOption wouldn't work.

@diegohaz diegohaz requested review from ntsekouras and ciampo October 12, 2021 12:11
@diegohaz diegohaz self-assigned this Oct 12, 2021
@diegohaz diegohaz added [Package] Components /packages/components [Type] Bug An existing feature does not function as intended labels Oct 12, 2021
@diegohaz diegohaz requested a review from mirka October 12, 2021 12:14
ntsekouras
ntsekouras approved these changes Oct 12, 2021
View reviewed changes
Copy link
Contributor

@ntsekouras ntsekouras 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 fix!

@ciampo
Copy link
Contributor

ciampo commented Oct 12, 2021

Out of curiosity, would using React.forwardRef be a cleaner solution instead of having a prop called forwardedRef?

@diegohaz
Copy link
Member Author

Since this component is not exposed, I'm okay with the forwardedRef prop. But we can refactor it so all the logic is inside ToggleGroupControlOption. And then delete ToggleGroupControlButton to simplify the code.

@ciampo
Copy link
Contributor

ciampo commented Oct 13, 2021

Since this component is not exposed, I'm okay with the forwardedRef prop. But we can refactor it so all the logic is inside ToggleGroupControlOption. And then delete ToggleGroupControlButton to simplify the code.

Makes sense.

We should merge this PR to get the fix in the codebase and then open a follow-up PR to merge ToggleGroupControlButton into ToggleGroupControlOption (let me know if you'd prefer to work on it directly, otherwise I can work on it myself).

@diegohaz diegohaz merged commit c906e26 into trunk Oct 13, 2021
@diegohaz diegohaz deleted the fix/toggle-group-control-option-ref branch October 13, 2021 10:45
@github-actions github-actions bot added this to the Gutenberg 11.8 milestone Oct 13, 2021
@ciampo
Copy link
Contributor

ciampo commented Oct 13, 2021 •
edited
Loading

We should merge this PR to get the fix in the codebase and then open a follow-up PR to merge ToggleGroupControlButton into ToggleGroupControlOption

I will take care of this and open a PR soon

Update: Opened #35600

@ciampo ciampo mentioned this pull request Oct 13, 2021
Merged
7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntsekouras ntsekouras ntsekouras approved these changes

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

@chrisvanpatten chrisvanpatten Awaiting requested review from chrisvanpatten

@ciampo ciampo Awaiting requested review from ciampo

@mirka mirka Awaiting requested review from mirka

Assignees

@diegohaz diegohaz

Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Milestone
Gutenberg 11.8
Development

Successfully merging this pull request may close these issues.
3 participants
@diegohaz @ciampo @ntsekouras