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

Buttons: Add a font size control #27134

Closed
wants to merge 1 commit into from

Conversation

pablinos
Copy link
Member

Description

In #20967 we have discussed adding a font size control to the button
block. This explores doing that, by adding fontSize to the block's
supported features.

Unfortunately, in testing with the TwentyTwenty theme, the size is
overridden by the theme's CSS. I've reset this in the block's CSS but
it would probably require theme updates to set the style on
.wp-block-button rather than .wp-block-button__link in order to make
sure this doesn't do something unexpected.

As it stands the default button size will have increased. I'm not sure if it
has other consequences.

How has this been tested?

Just manually for now with the TwentyTwenty theme

Screenshots

image

Types of changes

This is essentially a new feature. Introducing this capability to an existing block.

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.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@Soean Soean added the [Block] Buttons Affects the Buttons Block label Nov 20, 2020
@pablinos
Copy link
Member Author

I've just seen I missed another implementation of this that also adds support for padding controls in #26501.

I'm not sure if we should be adding the styles to the link inside the button, but that does get around the issue I was seeing where the theme is setting the font size of the link.

@skorasaurus
Copy link
Member

Thanks for sharing; does this come with the ability to disable that font-size control as well?

@pablinos
Copy link
Member Author

pablinos commented Dec 3, 2020

Thanks for sharing; does this come with the ability to disable that font-size control as well?

Do you mean to disable it? If so, then you could hook into blocks.registerBlockType and modify the supports object in the settings.

Copy link
Contributor

@apeatling apeatling left a comment

Choose a reason for hiding this comment

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

Works well and this combined with padding provides much greater button flexibility. 👍

@apeatling
Copy link
Contributor

apeatling commented Dec 10, 2020

I think you'll need to update tests first @pablinos

@apeatling
Copy link
Contributor

@pablinos Any plans to pick this one back up or would you prefer we take it over the line?

@pablinos
Copy link
Member Author

I can take a look at the tests today @apeatling. Do you think we should take this approach or go in the direction of #26501? Either way, we should add the padding controls as you mention.

Base automatically changed from master to trunk March 1, 2021 15:44
@aristath
Copy link
Member

aristath commented Mar 5, 2021

PR should be rebased to fix the merge conflicts 👍

In WordPress#20967 we have discussed adding a font size control to the button
block. This explores doing that, by adding `fontSize` to the block's
supported features.

Unfortunately, in testing with the TwentyTwenty theme, the size is
overridden by the theme's CSS. I've reset this in the block's CSS but
it would probably require theme updates to set the style on
`.wp-block-button` rather than `.wp-block-button__link` in order to make
sure this doesn't do something unexpected.

As it stands the default button size will have increased.
@pablinos pablinos force-pushed the add/button-font-size branch from 6e37200 to ea08c91 Compare March 26, 2021 12:38
@pablinos
Copy link
Member Author

Finally got around to rebasing this and the tests are passing now. What did you think about my comment here @apeatling? I'm not sure whether changing the default font size for a button is a good thing!

@aristath
Copy link
Member

cc @carolinan I don't know if TwentyTwenty-One should be changed if we land this... 🤔

@ntsekouras
Copy link
Contributor

Thanks for your work here @pablinos ! It seems though that this has slipped between the cracks and this landed: #30394 and latter updated as well with the new format for typography support.

I'm closing this but again thanks for your work - really appreciate it!

@ntsekouras ntsekouras closed this Jun 11, 2021
@pablinos
Copy link
Member Author

No problem @ntsekouras, glad it's been implemented!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Buttons Affects the Buttons Block
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants