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

Blocks: Revert the default fonti size changes for Paragraph #6075

Merged
merged 1 commit into from
Apr 9, 2018

Conversation

gziolo
Copy link
Member

@gziolo gziolo commented Apr 9, 2018

Description

As noted by @jorgefilipecosta in #5995:

We are setting the default size for font Size as regular making the range control start in some value. But we don't apply the regular class.
This may cause problems, imagine for example I set a CSS rule for paragraphs nested inside the cover image to have size 20px. These changes make the default size for paragraph appear as 16px but we don't set the class so the size will be in fact the one set with CSS for paragraphs inside cover image 20px.
Essentially I think there is a difference of state between not having size specified, no class et all where we use the default css rules depending on the context, and having regular font size where we use the regular class.

This PR reverts this behavior. I tried a few different tricks, but they all have some flaws. The most promising one was to set 16px only when it matches the computed style, but it doesn't work properly when switching back from HTML mode. In addition, it isn't consistent when you set the value back to 16px. Then it adds class name which isn't perfect. We can tackle it later in a separate PR.

How Has This Been Tested?

Manually. This is how font size setting should look like on the initial load for the default Paragraph block:

screen shot 2018-04-09 at 11 44 07

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code has proper inline documentation.

@gziolo gziolo added the [Feature] Blocks Overall functionality of blocks label Apr 9, 2018
@gziolo gziolo self-assigned this Apr 9, 2018
@gziolo gziolo added this to the 2.7 milestone Apr 9, 2018
@@ -190,7 +188,7 @@ class ParagraphBlock extends Component {
<RangeControl
className="blocks-paragraph__custom-size-slider"
label={ __( 'Custom Size' ) }
value={ fontSize }
value={ fontSize || '' }
Copy link
Member

Choose a reason for hiding this comment

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

Minor: I think the || ''may not be necessary.

Copy link
Member Author

Choose a reason for hiding this comment

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

It was there before so let's leave it as it is until you find a proper fix :D

Copy link
Member

@jorgefilipecosta jorgefilipecosta left a comment

Choose a reason for hiding this comment

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

Thank you for coming back to this, it tests well 👍 I'm trying an approach to improve the slider initial position using fallback styles it may just increase UX does not change this logic and does not conflict with this revert :)

@gziolo
Copy link
Member Author

gziolo commented Apr 9, 2018

@jorgefilipecosta, awesome. Thanks for taking care of it and catching this early 👍

@gziolo gziolo merged commit bf29d57 into master Apr 9, 2018
@gziolo gziolo deleted the update/paragraph-default-size branch April 9, 2018 11:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Blocks Overall functionality of blocks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants