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.

Sign up for GitHub

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
Jump to bottom

Navigation: Add font size attribute #19127

Merged
merged 4 commits into from Dec 19, 2019
Merged

Navigation: Add font size attribute #19127

merged 4 commits into from Dec 19, 2019

Conversation

Copons
Copy link
Contributor

@Copons Copons commented Dec 13, 2019 •
edited

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 added the [Block] Navigation Affects the Navigation Block label Dec 13, 2019
@Copons Copons requested a review from getdave December 13, 2019 14:33
@Copons Copons self-assigned this Dec 13, 2019
@Copons Copons added this to 👀 PRs to review in Navigation editor via automation Dec 13, 2019
@Copons
Copy link
Contributor Author

Copons commented Dec 13, 2019 •
edited

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
Merged
6 tasks
@Copons Copons force-pushed the try/navigation-block-style-attributes branch from 719afa0 to 616a07e Compare December 13, 2019 15:50
@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 December 16, 2019 15:17
noahtallen
noahtallen approved these changes Dec 17, 2019
View reviewed changes
Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

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

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

packages/block-library/src/navigation/edit.js Show resolved Hide resolved
@Copons Copons added the [Status] In Progress Tracking issues with work in progress label Dec 17, 2019
@Copons
Copy link
Contributor Author

Copons commented Dec 17, 2019 •
edited

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. 🙂

@Copons Copons added [Type] Enhancement A suggestion for improvement. and removed [Status] In Progress Tracking issues with work in progress labels Dec 17, 2019
@noahtallen
Copy link
Member

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

@noahtallen
Copy link
Member

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
Copy link
Contributor Author

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

talldan
talldan approved these changes Dec 18, 2019
View reviewed changes
Copy link
Contributor

@talldan talldan left a comment

Choose a reason for hiding this comment

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

This is working nicely, thanks for the addition.

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

packages/block-library/src/navigation/edit.js Outdated
@@ -175,9 +183,15 @@ function Navigation( {
>
<BlockNavigationList clientId={ clientId } />
</PanelBody>
<PanelBody title={ __( 'Text Settings' ) } className="blocks-font-size">
Copy link
Contributor

Choose a reason for hiding this comment

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

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' ) }>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Navigation editor automation moved this from 👀 PRs to review to ✅ Done Dec 19, 2019
@Copons Copons deleted the try/navigation-block-style-attributes branch December 19, 2019 12:52
@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
Reviewers

@talldan talldan talldan approved these changes

@noahtallen noahtallen noahtallen approved these changes

@getdave getdave Awaiting requested review from getdave

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@jorgefilipecosta jorgefilipecosta Awaiting requested review from jorgefilipecosta

@Soean Soean Awaiting requested review from Soean

@retrofox retrofox Awaiting requested review from retrofox

Assignees

@Copons Copons

Labels
[Block] Navigation Affects the Navigation Block [Type] Enhancement A suggestion for improvement.
Projects
No open projects
Navigation editor
  
Done
Milestone
Gutenberg 7.2
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@Copons @noahtallen @talldan @youknowriad