Skip to content

Conversation

@Clarity-89
Copy link

Description

Fixes #18211
In FontSizePicker changes made via slider or text input weren't reflected in the font size dropdown.
This was happening because FontSizePicker's font size dropdown was copying selected value from props onto state, therefore it was not properly updated when the props changed. By removing the state
and deriving it directly from props in render ensures the Dropdown has up to date value.

How has this been tested?

  1. Run Storybook: npm run design-system:dev
  2. Go to the FontSizePicker section.
  3. Select withSlider
  4. Drag the slider or select font size from input to change font size value.
  5. Observe that both select dropdown and input values are updating to reflect slider's value

Screenshots

slide1
slide2
slide3

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Font size select dropdown was copying selected value from props onto state,
therefore it was not properly updated when the props changed. By removing the state
and deriving it directly from props in render ensures the Dropdown has up
to date value.
@talldan talldan added [Feature] UI Components Impacts or related to the UI component system [Package] Components /packages/components First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Storybook Storybook and its stories for components labels Nov 1, 2019
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

@Clarity-89 Thanks for your contribution!

If I remember from the original review, the state was needed for a reason. Testing this branch in the storybook it looks like selecting 'Custom' from the <select> element has no effect, so perhaps that's the reason. I think that would need to be solved before this can be merged.

This component may also change in the future if the changes from #17926 are merged, so that's another thing to look out for.

@Clarity-89
Copy link
Author

@talldan thank you for the comments!

I indeed missed the Custom change behavior. After looking into it closer, it seems that selecting Custom does not have any other effect than changing the value in the dropdown, while selected font size stays the same. Should selecting Custom then change the font size to some value which is not in the list of provided fontSizes? That way the issue could be still fixed without using local component state.

Regarding #17926, it seems like the prop signature of CustomSelect introduced there is quite different from the current one, so if it were to used in FontSizePicker, the latter would have to be adjusted in any case.

@talldan
Copy link
Contributor

talldan commented Nov 6, 2019

@Clarity-89 I think the main thing is that when the custom option is selected it retains selection. I'm not sure what custom size it could choose, that would seem quite arbitrary. It's definitely a tricky behaviour, not one that's particularly elegant.

It may just be worth focusing efforts on getting #17926 merged, and then re-evaluating it afterwards. What do you think?

@Clarity-89
Copy link
Author

I'm not sure what custom size it could choose, that would seem quite arbitrary.

@talldan that was my opinion as well, not sure how to make custom selection in that case act in a non-arbitrary way.

I agree that we will probably have to come back to this functionality after #17926 is merged, but in the meanwhile do you think it would make sense to have a quick fix for the buggy behavior of the slider/select dropdown? I can modify the PR to use useEffect hook in order to track the changed font value across all internal input components.

@gziolo
Copy link
Member

gziolo commented Apr 28, 2020

@Clarity-89 thank you for working on this bug fix. In the meantime, the root issue was fixed so we can close this PR.

@gziolo gziolo closed this Apr 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Feature] UI Components Impacts or related to the UI component system First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Package] Components /packages/components Storybook Storybook and its stories for components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Font Size Picker: The selected preset doesn't change to Custom when picking a random value

3 participants