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

Centred Logo: Primary menu is slightly off #884

Merged
merged 2 commits into from
Apr 28, 2020
Merged

Centred Logo: Primary menu is slightly off #884

merged 2 commits into from
Apr 28, 2020

Conversation

laurelfulford
Copy link
Contributor

All Submissions:

Changes proposed in this Pull Request:

Right now when you have a centred logo and standard header height, the primary menu is slightly off-center. This is because the last menu item had a right margin, and if the last menu item has a dropdown, the empty space around the down-chevron svg also falsely makes it look off.

Closes #883.

How to test the changes in this Pull Request:

  1. Set up a primary menu, and add dropdown items to the last menu item.
  2. Navigate to Customize > Header Settings, and check Center Logo and uncheck Short Header.
  3. View on the front-end; you may need to take a screenshot and measure, but the primary menu has about a 10px offset to the left:

centered-logo-before

  1. Apply the PR and run npm run build.
  2. Confirm that the primary menu is more properly centred:

centered-logo-after

  1. Try moving the menu item with the dropdown away from the last item, and confirm that it still looks centred with a one level menu item at the end:

centered-logo-after-2

Other information:

  • Have you added an explanation of what your changes do and why you'd like us to include them?
  • Have you written new tests for your changes, as applicable?
  • Have you successfully ran tests with your changes locally?

@laurelfulford laurelfulford added [Status] Needs Review The issue or pull request needs to be reviewed Smaller PR This PR is smaller and quick to test. labels Apr 15, 2020
@jeffersonrabb jeffersonrabb self-requested a review April 20, 2020 13:47
jeffersonrabb
jeffersonrabb suggested changes Apr 20, 2020
View reviewed changes
Copy link
Contributor

@jeffersonrabb jeffersonrabb left a comment

Choose a reason for hiding this comment

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

When I set up my site to test this I found the logo was dramatically off center, and remained that way after switching to this branch.

screenshot

Theme settings:

  • Logo size: Smallest possible
  • Display Site Title/Display Tagline: Off
  • Center Logo: On
  • Solid Background: Off
  • Short Header: Off

@jeffersonrabb jeffersonrabb added [Status] Needs Changes or Feeback The issue or pull request needs action from the original creator and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Apr 20, 2020
@laurelfulford
Copy link
Contributor Author

Thanks Jeff!

I haven't had any luck recreating this -- I've tried as similar settings as possible with the menus, custom fonts and theme, and tried with Firefox and Chrome, with and without AMP:

image

Could you please "inspect" your test site and share the styles that are being added to the logo area? There should be a text-align: center that's making this work:

image

@laurelfulford laurelfulford added [Status] Needs Review The issue or pull request needs to be reviewed and removed [Status] Needs Changes or Feeback The issue or pull request needs action from the original creator labels Apr 20, 2020
@laurelfulford
Copy link
Contributor Author

Just to close loops: #887 should resolve the issue with the logo not centering.

@jeffersonrabb jeffersonrabb added [Status] Approved The pull request has been reviewed and is ready to merge and removed [Status] Needs Review The issue or pull request needs to be reviewed labels Apr 28, 2020
@laurelfulford
Copy link
Contributor Author

Thanks Jeff!

@laurelfulford laurelfulford merged commit 9897cb1 into master Apr 28, 2020
@laurelfulford laurelfulford deleted the fixes/883-offset-centred-menu branch April 28, 2020 18:49
matticbot pushed a commit that referenced this pull request Apr 29, 2020
@semantic-release-bot
chore(release): 1.6.1 [skip ci]
ddbd421 
## [1.6.1](v1.6.0...v1.6.1) (2020-04-29)

### Bug Fixes

* add a temporary fix for a doubled block button outline ([#907](#907)) ([57df314](57df314))
* add color selectors for the editor ([#898](#898)) ([6e94411](6e94411))
* improve appearance of caption with beside featured image setting ([#882](#882)) ([1da6e44](1da6e44))
* make sure amp styles work with logo centering ([#887](#887)) ([0aaec4c](0aaec4c))
* make sure first floated element has top margin ([#903](#903)) ([733d482](733d482))
* tweak primary menu to be properly centred ([#884](#884)) ([9897cb1](9897cb1))
@matticbot
Copy link
Contributor

🎉 This PR is included in version 1.6.1 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeffersonrabb jeffersonrabb jeffersonrabb approved these changes

Assignees
No one assigned
Labels
released Smaller PR This PR is smaller and quick to test. [Status] Approved The pull request has been reviewed and is ready to merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Centred Logo: Not quite centered when compared to primary menu
3 participants
@laurelfulford @matticbot @jeffersonrabb