-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Dropdown Items in Navbar #13
Conversation
docs/default-theme-config/README.md
Outdated
@@ -48,6 +48,25 @@ module.exports = { | |||
} | |||
``` | |||
|
|||
The item in `themeConfig.nav` could also be a dropdown menu: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those links can also be dropdown menus, add nested items via items
:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed at 50689ea, thanks
lib/default-theme/NavLink.vue
Outdated
<router-link | ||
class="router-link" | ||
:to="item.link" | ||
v-if="!isExternal(item.link)"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
better put the >
on the next line, stuck to {{ item.text }}
and stick the next router-link
to the end of it too. But I see it comes this way from the original code 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, when I checked existing code, I found that the original code is always this way:
<!-- lib/default-theme/SidebarGroup.vue -->
<span class="arrow"
v-if="collapsable"
:class="open ? 'up' : 'down'"></span>
And Is that what you say?
<router-link
class="router-link"
:to="item.link"
v-if="!isExternal(item.link)"
>{{ item.text }}</router-link>
But I cannot find this style at existing code. or these two following styles would be better? it's most likely to original style.
<router-link class="router-link"
:to="item.link"
v-if="!isExternal(item.link)">
{{ item.text }}
</router-link>
or
<router-link class="router-link"
:to="item.link"
v-if="!isExternal(item.link)">{{ item.text }}</router-link>
Can you give a code snippet that you think is reasonable? Thank you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's pretty much what I said
The reasonable one would be:
<router-link
class="router-link"
:to="item.link"
v-if="!isExternal(item.link)"
>{{ item.text }}</router-link>
which doesn't create any extra white space around the text, but, let's wait for Evan input on this one
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
cool, fixed at 7eb2563
lib/default-theme/NavLink.vue
Outdated
v-else | ||
:href="item.link" | ||
target="_blank" | ||
class="router-link"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here
lib/default-theme/NavLink.vue
Outdated
required: true | ||
} | ||
}, | ||
mixins: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you use a mixin here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, using a mixin here is to reuse the existing method. or you would have a better way to do it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you can just put the methods object without the mixins, it would also work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, long time to write Vue, I just forgot some important thing, thanks.
lib/default-theme/NavLinks.vue
Outdated
return (this.$site.themeConfig.nav || []).map(({ text, link, type, items }) => ({ | ||
text, | ||
type, | ||
link: link ? ensureExt(link) : void 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe something more readable like null
instead of void 0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed at 50689ea, thanks
lib/default-theme/NavLinks.vue
Outdated
top calc(50% - 3px) | ||
left -10px | ||
&:hover | ||
margin-bottom 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should be moved to a
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
AWESOME! fixed at 6de2efa, thank for finding it out.
lib/default-theme/NavLinks.vue
Outdated
&:after | ||
content: "" | ||
width: 0 | ||
height: 0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no :
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed at 50689ea, thanks
I just checked at vue official doc, and found out this dropdown style, @yyx990803 Do you think I need to finish this feature in this PR? (In To do that, I just come up with some config styles for that:
module.exports = {
themeConfig: {
nav: [
{
text: 'Ecosystem',
type: 'dropdown',
items: [
{ text: 'Help', type: 'label' }, // Or allow to omit the type?
{ text: 'Forum', link: '...' },
{ text: 'Chat', link: '...' },
{ text: 'Tooling' },
{ text: 'Devtools', link: '...' }
// ...
]
}
]
}
}
module.exports = {
themeConfig: {
nav: [
{
title: 'Ecosystem',
type: 'dropdown',
items: [
{ type: 'label', title: 'Help' },
// ... items
{ type: 'sep' } // separator
// ... other items
]
}
]
}
}
module.exports = {
themeConfig: {
nav: [
{
text: 'Ecosystem',
type: 'dropdown',
items: [
{
text: 'Help',
items: [
{ text: 'Forum', link: '...' },
{ text: 'Chat', link: '...' }
]
},
{
text: 'Tooling',
items: [
{ text: 'Devtools', link: '...' },
// ...
]
},
// ...
]
}
]
}
} Which do you think is better? or better suggestion? BTW, at present, when a item does not provide |
@posva Addressed all comments, thanks. |
I like style 3 (Semantic). We can treat |
@yyx990803 Got it, I will work at here. In addition, I am also working for the PR is here. https://github.com/ulivz/vuepress/pull/1, now the basic function is done and the rest of the job is almost translating, welcome to go here and give me some thoughts so that I can make it better. thank you. |
UpdatedCommit: 6c9e1a4
|
lib/default-theme/NavLink.vue
Outdated
class="router-link" | ||
:to="link" | ||
v-if="!isExternal(link)" | ||
exact |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 67c758e, should only be exact
for the homepage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed at 96a6cc6.
lib/default-theme/NavLinks.vue
Outdated
<li | ||
v-for="childSubItem in subItem.items" | ||
:key="childSubItem.link"> | ||
<nav-link :item="childSubItem"></nav-link> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some conflicts due to fixes which are also addressed in this PR. Should be easy to resolve.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All conflict has been resolved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good! Thanks for the great work. Some final small tweaks:
-
.nav-item
should havecursor: pointer
-
On mobile, the dropdown links should be togglable. Otherwise with a lot of dropdown items, the sidebar of the current page gets pushed down too much.
-
To implement (2) we should probably extract the dropdown into its own component.
@yyx990803 As a fanatic fan of Vue, it's voluntary and my honor to do this. Thank you for your advices. It looks better now. |
Awesome! One final thing: the dropdowns should be collapsed by default on Mobile. After that we can remove the test dropdown links and ready to merge. |
Fixed at b3e0583 ~ |
Thanks for the great work! 🎉 |
Summary
This branch is for the dropdown Items in navbar, and fully leverage the style from vue offical website.
Changes
**guide/https://vuejs.org/**
.Prewview