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

fix(navigation-block): reduce subnavigation arrow padding #24200

Merged
merged 3 commits into from Sep 11, 2020
Merged

fix(navigation-block): reduce subnavigation arrow padding #24200

merged 3 commits into from Sep 11, 2020

Conversation

devfle
Copy link
Contributor

@devfle devfle commented Jul 24, 2020 •
edited

Description

I currently work very intensively with full site editing. In the navigation block I noticed that the padding between the subnavigation point and the arrow icon is much too large. I have now adjusted this in the editor and on the published page.

How has this been tested?

I have built the Gutenberg plugin and installed it into my local wordpress installation. Afterwards I checked my changes in different browsers and could not find any errors. I tested it with many, few and long navigation points. I also checked on mobile viewports.

  1. Build Gutenberg Plugin
  2. Inserted Navigation Block
  3. Added many, few and long nav points
  4. testet on diffrent browsers
  5. resized browser / navigation few times

Screenshots

Before:
before-site

before-editor

After:
after-website

after-editor

Types of changes

Bug fix (non-breaking change which fixes an issue)

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.

@devfle
Copy link
Contributor Author

Edit: I tested both navigation blocks -> horizontal and vertical.

@devfle devfle changed the title Fix/subnavigation arrow padding fix(navigation-block): reduce subnavigation arrow padding Aug 5, 2020
@devfle devfle force-pushed the fix/subnavigation-arrow-padding branch from ae5a1ad to e72c60d Compare August 5, 2020 14:57
@ZebulanStanphill ZebulanStanphill added [Block] Navigation Affects the Navigation Block CSS Styling Related to editor and front end styles, CSS-specific issues. Needs Design Feedback Needs general design feedback. labels Aug 28, 2020
@devfle
Copy link
Contributor Author

Hi @talldan, @Soean, @ajitbohra - Just wanted to ask if someone can have a look. :-) Thank you!

tellthemachines
tellthemachines reviewed Sep 9, 2020
View reviewed changes
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

The units used for padding have changed since this PR was opened: we're now using em instead of px. That's what is causing the merge conflicts btw 😅 . Best approach is to rebase master and then convert your changes to em.

packages/block-library/src/navigation-link/style.scss Outdated
@@ -29,6 +29,9 @@
// Styles for submenu flyout
.has-child {
$navigation-vertical-padding: $grid-unit-10 * 0.75;
> a {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you target .wp-block-navigation-link__content instead of a in this file, it will apply to both editor and front end, so you won't need the change in editor.scss 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you very much! I will work on it 😄 !

@devfle devfle force-pushed the fix/subnavigation-arrow-padding branch from e72c60d to 709d88e Compare September 10, 2020 07:38
@devfle
Copy link
Contributor Author

@tellthemachines Thanks a lot for your help. I rebased the project, converted the px values to em values and removed the redundant code.

I have tested the whole thing again. Looks good so far.

tellthemachines
tellthemachines approved these changes Sep 11, 2020
View reviewed changes
Copy link
Contributor

@tellthemachines tellthemachines left a comment

Choose a reason for hiding this comment

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

Thanks for the quick turnaround! Looks good 🚀

kevin940726
kevin940726 reviewed Sep 11, 2020
View reviewed changes
packages/block-library/src/navigation-link/style.scss Outdated
@@ -28,6 +28,10 @@

// Styles for submenu flyout
.has-child {
$navigation-vertical-padding: $grid-unit-10 * 0.75;
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, why do we need this 🤔 ? Is it used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @kelin1003 The variable is actually no longer needed. It was also removed in the previous PR:
bc77362

I have now removed it too and tested everything again. So far everything looks good. Thanks!

@draganescu draganescu merged commit 162ce54 into WordPress:master Sep 11, 2020
@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Sep 11, 2020
@github-actions
Copy link

Congratulations on your first merged pull request, @devfle! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

@github-actions github-actions bot added this to the Gutenberg 9.0 milestone Sep 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tellthemachines tellthemachines tellthemachines approved these changes

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@Soean Soean Awaiting requested review from Soean

@talldan talldan Awaiting requested review from talldan

@noisysocks noisysocks Awaiting requested review from noisysocks

@draganescu draganescu Awaiting requested review from draganescu

@kevin940726 kevin940726 Awaiting requested review from kevin940726

Assignees
No one assigned
Labels
[Block] Navigation Affects the Navigation Block CSS Styling Related to editor and front end styles, CSS-specific issues. First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Needs Design Feedback Needs general design feedback.
Projects
None yet
Milestone
Gutenberg 9.0
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@devfle @kevin940726 @tellthemachines @draganescu @ZebulanStanphill