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

Forward root element styles to mobile navigation menu #1562

Merged
merged 5 commits into from
May 5, 2021

Conversation

ang-zeyu
Copy link
Contributor

@ang-zeyu ang-zeyu commented May 2, 2021

What is the purpose of this pull request?

Goes on top of #1561!
Relevant commit here is 205426e

Overview of changes:

  • Forward class and style of the root element to the mobile navigation menu as well
  • Edit a small style in the docs / test / template site stylesheets to accomodate this

Anything you'd like to highlight / discuss:
na

Testing instructions:

<div id="site-nav" class="some-custom-class" style="and this">
...
</div>

Should work

Proposed commit message: (wrap lines at 72 characters)
Forward root element styles to mobile navigation menu


Checklist: ☑️

  • Updated the documentation for feature additions and enhancements
  • Added tests for bug fixes or features
  • Linked all related issues
  • No blatantly unrelated changes
  • Pinged someone for a review!

@ang-zeyu ang-zeyu requested a review from jonahtanjz May 2, 2021 12:55
Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

Nice work and it looks good @ang-zeyu :)

With this change, the updated page nav will have a longer grey border compared to the previous implementation.

image

I wonder if we should hide the border (on desktop) if the page nav is empty now that it is more apparent? 🤔

@ang-zeyu ang-zeyu force-pushed the forward-mobile-nav-styles branch from 988c785 to 96594fb Compare May 5, 2021 07:34
@ang-zeyu ang-zeyu removed the s.OnHold label May 5, 2021
@ang-zeyu
Copy link
Contributor Author

ang-zeyu commented May 5, 2021

Thanks for the review and catch! @jonahtanjz

I've fixed it by adding align-items: start to the flex container.
The default https://developer.mozilla.org/en-US/docs/Web/CSS/align-items was causing it stretch instead.

There's still a small little amount of border left, but this is an existing separate issue due to the .nav-component element having some padding. It requires some shifting of where the paddings of the containers are applied. I'll fix it separately!

Untitled

Copy link
Contributor

@jonahtanjz jonahtanjz left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jonahtanjz jonahtanjz merged commit d8066fb into MarkBind:master May 5, 2021
@jonahtanjz jonahtanjz added this to the v3.0 milestone May 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants