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.

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

[Components - ToggleGroupControl]: Fix extra update on incoming change #37224

Merged

Conversation

ntsekouras
Copy link
Contributor

@ntsekouras ntsekouras commented Dec 8, 2021

Fixes: #37172

There was a bug in ToggleGroupControl component where it would call the passed onChange function during rendering an extra time where the value was the result of an incoming change (redo/undo). That would cause an extra level in history which was the same with the previous one, due to internal logic of handling history steps.

Testing instructions (from the issue 😄 )

  1. Navigate to the post editor
  2. Add text to a paragraph block
  3. In the paragraph block's block settings menu, update line height
  4. Update font size
  5. Undo until the text and typography updates are gone
  6. Redo until you return to the current revision
  7. Verify that font-size isn't restored

** noting that to reproduce the bug we have to also have at least one more typography setting which is handled by style object property. This is needed because it has to do with internals detection of marking history steps.

Before

Screen.Recording.2021-12-08.at.4.27.48.PM.mov

After

after.mov

@ntsekouras ntsekouras added [Type] Bug An existing feature does not function as intended [Package] Components /packages/components labels Dec 8, 2021
@ntsekouras ntsekouras requested a review from ajitbohra as a code owner December 8, 2021 14:44
@ntsekouras ntsekouras self-assigned this Dec 8, 2021
Copy link
Contributor

@mcsf mcsf left a comment

Choose a reason for hiding this comment

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

Looks good, although the manual two-way binding with the twin useUpdateEffect hooks is something I found surprising.

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.

The changes test well as per instructions, and do fix the issue reported in #37172

I just wonder if this change in behavior could be unexpected for a consumer of this component, in case they expected the onChange callback to fire every time there's a change in value, regardless of whether the change is "external" or "internal"

In any case, could we also add a couple of doc changes:

  • potentially update the onChange prop docs in the README and in types.ts specifying the new behaviour
  • add a CHANGELOG entry to the @wordpress/components package? It can go under the Experimental section, since the component is marked as such

@youknowriad
Copy link
Contributor

I just wonder if this change in behavior could be unexpected for a consumer of this component, in case they expected the onChange callback to fire every time there's a change in value, regardless of whether the change is "external" or "internal"

I think we should base the behavior of all the components on the behavior of the regular elements (input...) and onChange is not called there if it's an external change.

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.

I think we should base the behavior of all the components on the behavior of the regular elements (input...) and onChange is not called there if it's an external change.

That sounds reasonable, let's go ahead and merge these changes once the CHANGELOG is updated

@ntsekouras ntsekouras merged commit 52012ee into trunk Dec 9, 2021
@ntsekouras ntsekouras deleted the fix/toggle-group-control-extra-update-on-incoming-change branch December 9, 2021 11:25
@github-actions github-actions bot added this to the Gutenberg 12.2 milestone Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Components /packages/components [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Editor History: Font size settings don't redo properly
4 participants