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

Stop displaying FontSizePicker dropdown when there are no fonts. #13782

Merged
merged 2 commits into from Feb 11, 2019

Conversation

@Naerriel
Copy link
Contributor

Naerriel commented Feb 8, 2019

Description

Closes #11628.
The FontSizePicker currently displays a dropdown with fonts, even when there are no fonts.
This change stops displaying the dropdown in this case.

How has this been tested?

In packages/components/src/font-size-picker/index.js I overwrote fontSizes to [] and the dropdown would not display anymore.
Ran npm run test and see that all tests passes.

Screenshots

image

Types of changes

Bug fix.

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.
@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Feb 8, 2019

@Naerriel What was the PHP you used to set the font sizes to empty?

Merge branch 'master' of https://github.com/WordPress/gutenberg into …
…fix/font-picker-present-with-no-fonts
@Naerriel

This comment has been minimized.

Copy link
Contributor Author

Naerriel commented Feb 9, 2019

@chrisvanpatten Excellent question. I haven't used the PHP, I set it in JS.
I forgot about that part of the issue. I don't know how to do that and whether it is even possible.

Possible solutions:

  1. Modify the default WordPress behaviour to allow disabling editor font sizes (has probably no use case).
  2. Close the Pull Request.

I'm not sure if I have got the call to action reached in #11628 correctly as well.
I could delete the Dropdown alongside the reset button, if there is one editor font size and the custom font sizes are disabled, which was the expected behaviour posed by the original poster.

What do you think?

@chrisvanpatten

This comment has been minimized.

Copy link
Member

chrisvanpatten commented Feb 9, 2019

@Naerriel we do have a PHP function for pre-defining the dropdown sizes, add_theme_support( 'editor-font-sizes', (array) $sizes ); but from what I don't know if it's possible to set an empty array. If that is possible, and works in combination with this PR, it seems like a great fix. If it isn't possible it might be worth a core patch to allow that behavior so this can land.

@Naerriel

This comment has been minimized.

Copy link
Contributor Author

Naerriel commented Feb 9, 2019

@chrisvanpatten Yes, when you asked your question, I tried to do it in several ways but none worked.

I can patch this behaviour in the core, but I don't think people will use it. If you think that this should be done, I can get my hands on that 😄

@jorgefilipecosta
Copy link
Member

jorgefilipecosta left a comment

Thank you for your contribution @Naerriel and thank you for reviewing @chrisvanpatten 👍
It should be possible to use add_theme_support( 'editor-font-sizes', array() );. But there is a bug that is affecting this change. I will open a follow-up PR to address this problem.
The change in this PR makes total sense and fixes a bug where the dropdown renders incorrectly so independently of the other bug so I'm going to merge it and then we can continue the iteration.

@jorgefilipecosta jorgefilipecosta merged commit 0c30ee3 into WordPress:master Feb 11, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@nervewax

This comment has been minimized.

Copy link

nervewax commented Feb 13, 2019

@jorgefilipecosta Are you able to link to your pull/issue relating to the fix preventing add_theme_support( 'editor-font-sizes' ) from acting the same way as add_theme_support( 'editor-color-palette' )?

This issue was closed, however the original problem still exists.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Feb 25, 2019

mukeshpanchal27 added a commit to mukeshpanchal27/gutenberg that referenced this pull request Feb 26, 2019

youknowriad added a commit that referenced this pull request Mar 6, 2019

youknowriad added a commit that referenced this pull request Mar 6, 2019

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.