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

Fixes the FontSizePicker Custom option #18842

Conversation

@JeanDavidDaviet
Copy link
Contributor

JeanDavidDaviet commented Nov 30, 2019

Fixes #18716
This commit prevent the "Custom" option still showing up in the font sizes drop down when a WordPress theme uses the add_theme_support( 'disable-custom-font-sizes' ) feature.

Description

There was no check on the disableCustomFontSizesvariable. The custom object { slug: 'custom', name: __( 'Custom' ) } was then always added even when not wanted.

How has this been tested?

Tested with Storybook and npm test

Types of changes

Bug Fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
This commit prevent the "Custom" option still showing up in the font sizes drop down when  a WordPress theme uses the `add_theme_support( 'disable-custom-font-sizes' )` feature.
The slider didn't update the select options. In the new `onSliderChangeValue` function, we set the new fontSize state with the props function `onChange` and we also set the selection option given the new font size.
This reverts commit 73f952f.
I don't know why Travis doesn't accept this commit.
@JeanDavidDaviet JeanDavidDaviet requested a review from ZebulanStanphill Dec 2, 2019
Copy link
Contributor Author

JeanDavidDaviet left a comment

I don't understand why this change makes Travis build fail. @ZebulanStanphill ?
They are necessary to update the custom select when the fontsize slider is used.

@ZebulanStanphill

This comment has been minimized.

Copy link
Contributor

ZebulanStanphill commented Dec 3, 2019

@JeanDavidDaviet The automated tests haven't been working quite right lately, so the failure may be unrelated. Try recommitting the change.

This reverts commit aa62643.
@JeanDavidDaviet

This comment has been minimized.

Copy link
Contributor Author

JeanDavidDaviet commented Dec 3, 2019

@ZebulanStanphill Thanks, that was it !

@ZebulanStanphill ZebulanStanphill merged commit 68f0dc5 into WordPress:master Dec 3, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@epiqueras

This comment has been minimized.

Copy link
Contributor

epiqueras commented Dec 5, 2019

It would be good to add tests for this.

@JeanDavidDaviet

This comment has been minimized.

Copy link
Contributor Author

JeanDavidDaviet commented Dec 5, 2019

It would be good to add tests for this.

@epiqueras I won't be home for december, but I sure would like to see the test suite for this feature implemented (or modified) as I'm not a good test writer.

@JeanDavidDaviet JeanDavidDaviet deleted the JeanDavidDaviet:fix/custom-font-size-picker-18716 branch Dec 5, 2019
@youknowriad youknowriad added this to the Gutenberg 7.1 milestone Dec 9, 2019
scruffian added a commit to scruffian/gutenberg that referenced this pull request Dec 10, 2019
* Fixes the FontSizePicker Custom option

This commit prevent the "Custom" option still showing up in the font sizes drop down when  a WordPress theme uses the `add_theme_support( 'disable-custom-font-sizes' )` feature.

* Return directly the optionArray for the FontSizePicker instead of spread

* Update the fontSizePicker when font size slider is used

The slider didn't update the select options. In the new `onSliderChangeValue` function, we set the new fontSize state with the props function `onChange` and we also set the selection option given the new font size.

* Revert "Update the fontSizePicker when font size slider is used"

This reverts commit 73f952f.
I don't know why Travis doesn't accept this commit.

* Update the fontSizePicker when font size slider is used

This reverts commit aa62643.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.