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

Front End styling tidied up for Search Bar and Nav Links in the Header #266

Closed
wants to merge 4 commits into from

Conversation

samuelgoddard
Copy link

I noticed the nav in the header had a few small issues at narrow sizes (740px etc) where the Nav Links are overlapping the logo, and focusing the Search Bar breaks out of the layout.

I've reduced the size of the padding on the Nav Links and changed the styling of the focused Search Bar to absolutely position over the top of the Nav at Narrow breakpoints.

Let me know what you think. Cheers

@yyx990803
Copy link
Member

Can you add a before/after screenshot for easier review?

@samuelgoddard
Copy link
Author

Sure

Before (unfocused search)
before-unfocused
After (unfocused search)

after-unfocused

Before (focused search)
before-focused

After (focused search)
after-focused

When I get half an hour I'll put in a more solid fix as there's some other funky stuff going on with the nav when there's a tonne of nav items, it would be better to drop them onto another line that was it's infinitely scalable

Cheers

cursor text
width 10rem

@media (min-width: $MQMobile) and (max-width: $MQNarrow)
Copy link
Member

Choose a reason for hiding this comment

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

Currently there are multiple media queries with duplicated rules that can be merged together. e.g. This is the exact same media query as the one on line 223.

@@ -184,17 +184,40 @@ export default {
a
color $accentColor

@media (max-width: $MQNarrow)
@media (max-width: $MQMobile)
Copy link
Member

Choose a reason for hiding this comment

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

This should be merged with the media query on line 228

background-color white
z-index 10
left 0
top: 0
Copy link
Member

@meteorlxy meteorlxy Apr 30, 2018

Choose a reason for hiding this comment

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

remove the :

Copy link
Author

@samuelgoddard samuelgoddard left a comment

Choose a reason for hiding this comment

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

All look good

@samuelgoddard
Copy link
Author

@yyx990803 - changes pushed and ready for review

Copy link
Member

@ulivz ulivz left a comment

Choose a reason for hiding this comment

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

Some behaviors looks not very good, see:

Before

vuepress-266-before

After

vuepress-266-after

@ulivz ulivz force-pushed the master branch 3 times, most recently from 14d9013 to c7c0cd9 Compare June 8, 2018 19:11
@ulivz ulivz force-pushed the master branch 5 times, most recently from c992e38 to c2eaff3 Compare July 12, 2018 18:01
@ulivz ulivz force-pushed the master branch 4 times, most recently from 0c3bc3a to cf1e6fc Compare July 28, 2018 08:12
@chrisvfritz
Copy link

@michalsnik @shentao This is affecting a lot of Vue docs. Would either of you be able to help take over this PR and push it to completion? 🙂

@ulivz
Copy link
Member

ulivz commented Aug 1, 2018

@chrisvfritz Sorry for the delay, I have fixed this issue but I need more condition-coverage test, will try to release it at ASAP.

@chrisvfritz
Copy link

@ulivz Thank you! I appreciate your work. 🙂

@ulivz
Copy link
Member

ulivz commented Aug 8, 2018

Closing it since we had a better fix at #714.

@ulivz ulivz closed this Aug 8, 2018
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.

5 participants