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

Navigation: Add font size attribute #19127

Merged
merged 4 commits into from Dec 19, 2019
Merged

Conversation

@Copons
Copy link
Contributor

Copons commented Dec 13, 2019

Description

Adds the font size style attributes (both named and custom) to the Navigation block.

How has this been tested?

Tested on macOS 10.15.2, Chrome 78, Gutenberg Docker environment, Twenty Twenty theme.

Screenshots

Before After
Screenshot 2019-12-13 at 15 53 36 Screenshot 2019-12-13 at 15 54 02
Screenshot 2019-12-13 at 15 51 58 Screenshot 2019-12-13 at 15 54 22

Testing instructions

  • Edit a post and add a Navigation block.
  • Try changing the block's font size.
  • Make sure the block looks as expected.
  • Preview the post, and make sure the block looks as expected in the front end too.
  • Repeat the previous steps, but this time check if the font size custom attribute work as expected.

Types of changes

New feature (non-breaking change which adds functionality)

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. .
@Copons Copons requested a review from getdave Dec 13, 2019
@Copons Copons self-assigned this Dec 13, 2019
@Copons Copons added this to 👀 PRs to review in Navigation block via automation Dec 13, 2019
@Copons

This comment has been minimized.

Copy link
Contributor Author

Copons commented Dec 13, 2019

Ah, for crying out loud, I sat on this for too long and we already have #19108 open... 😓

I guess this is still valuable for the font size part though.

(Discussing at #19108 (comment) about dropping the first two commits of this PR, and only keep the font size parts)

@Copons Copons mentioned this pull request Dec 13, 2019
1 of 6 tasks complete
@Copons Copons force-pushed the try/navigation-block-style-attributes branch from 719afa0 to 616a07e Dec 13, 2019
@Copons Copons changed the title Navigation: Add font size and background color Navigation: Add font size attribute Dec 13, 2019
@Copons Copons requested a review from retrofox Dec 16, 2019
Copy link
Contributor

noahtallen left a comment

The code looks good to me. It also tests great locally!

One thought -- I notice the paragraph block uses a new inline mechanism for font. Should we add this too?

Screen Shot 2019-12-16 at 4 54 00 PM

@Copons

This comment has been minimized.

Copy link
Contributor Author

Copons commented Dec 17, 2019

The code looks good to me. It also tests great locally!

One thought -- I notice the paragraph block uses a new inline mechanism for font. Should we add this too?

Screen Shot 2019-12-16 at 4 54 00 PM

@noahtallen Oh, I missed that!
I'm putting this in WIP so that I can check it out and see how to integrate it here.

EDIT: Ah, never mind, that comes from CoBlocks. 🙂

@noahtallen

This comment has been minimized.

Copy link
Contributor

noahtallen commented Dec 17, 2019

oh my 😁 my bad! That's some good stuff. We should have that

@noahtallen

This comment has been minimized.

Copy link
Contributor

noahtallen commented Dec 17, 2019

Who should we ping on this? I think it should be ready to go. I can't think of any reason to not have it.

@Copons

This comment has been minimized.

Copy link
Contributor Author

Copons commented Dec 17, 2019

@noahtallen I'm not sure, but the codeowners have been automagically pinged already. 🙂

Copy link
Contributor

talldan left a comment

This is working nicely, thanks for the addition.

It'd be great to remove that one classname before merging.

@@ -175,9 +183,15 @@ function Navigation( {
>
<BlockNavigationList clientId={ clientId } />
</PanelBody>
<PanelBody title={ __( 'Text Settings' ) } className="blocks-font-size">

This comment has been minimized.

Copy link
@talldan

talldan Dec 18, 2019

Contributor

The blocks-font-size class doesn't seem to do anything, so I think it'd be good to remove it. I did a search of the codebase and it looks like it's only used in end-to-end tests in the paragraph block.

Update: I put together a PR (#19208) to remove it entirely from the codebase. It looks like there used to be styles for this class in the paragraph block, but they've been removed. The e2e tests were updated to use a different classname in that PR.

Suggested change
<PanelBody title={ __( 'Text Settings' ) } className="blocks-font-size">
<PanelBody title={ __( 'Text Settings' ) }>

This comment has been minimized.

Copy link
@Copons

Copons Dec 19, 2019

Author Contributor

Oh good catch! Removed in 6789b26.

I'll merge as soon as tests are clear!

@Copons Copons merged commit 9ddf124 into master Dec 19, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
Navigation block automation moved this from 👀 PRs to review to ✅ Done Dec 19, 2019
@Copons Copons deleted the try/navigation-block-style-attributes branch Dec 19, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
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.