Skip to content

Changing repo label depending on the host #168

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

Merged
merged 6 commits into from
Apr 20, 2018
Merged

Changing repo label depending on the host #168

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
34ccc58
fixing #167
ycmjason Apr 20, 2018
bb33ae3
added option to customise repoLabel
ycmjason Apr 20, 2018
a72e8a4
fixing styles
ycmjason Apr 20, 2018
0e18730
fixing non polifilled logic and unsafe match
ycmjason Apr 20, 2018
1a103a4
Update .gitignore
yyx990803 Apr 20, 2018
7fac79e
Update NavLinks.vue
yyx990803 Apr 20, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,3 +4,4 @@ node_modules
.temp
vuepress
TODOs.md
*.sw*
Copy link
Member

Choose a reason for hiding this comment

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

Why adding this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For vim swap files.

Copy link
Member

Choose a reason for hiding this comment

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

You should add it at your project instead of here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just thought it would be great since there might be other contributors who use vim.

Copy link
Member

Choose a reason for hiding this comment

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

Hmmm, so should I put all the different needs here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's up to you guys. I could remove it if you don't like it.

I just feel that it's not something that will hurt to add and might make other vim contributors lives easier. If there are other contributors who uses other editors which has some other ignore files, I couldn't see why we shouldn't facilitate it.

Copy link
Member

@ulivz ulivz Apr 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It doesn't depend on me. please do not kidnap thinking.

What I want to say it that you can set this to a global git configuration instead of duplicating it everywhere.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is certainly not true for me. I am happy to remove that if this is not wanted.

5 changes: 4 additions & 1 deletion docs/default-theme-config/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ next: false
---
```

## GitHub Repo and Edit Links
## Git Repo and Edit Links

Providing `themeConfig.repo` auto generates a GitHub link in the navbar and "Edit this page" links at the bottom of each page.

Expand All @@ -226,6 +226,9 @@ module.exports = {
themeConfig: {
// Assumes GitHub. Can also be a full GitLab url.
repo: 'vuejs/vuepress',
// Customising the header label
// Defaults to "GitHub"/"GitLab"/"Bitbucket" depending on `themeConfig.repo`
repoLabel: 'Contribute!',
// if your docs are not at the root of the repo
docsDir: 'docs',
// optional, defaults to master
Expand Down
28 changes: 18 additions & 10 deletions lib/default-theme/NavLinks.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
<template>
<nav class="nav-links" v-if="userLinks.length || githubLink">
<nav class="nav-links" v-if="userLinks.length || repoLink">
<!-- user links -->
<div
class="nav-item"
Expand All @@ -8,13 +8,13 @@
<DropdownLink v-if="item.type === 'links'" :item="item"/>
<NavLink v-else :item="item"/>
</div>
<!-- github link -->
<a v-if="githubLink"
:href="githubLink"
class="github-link"
<!-- repo link -->
<a v-if="repoLink"
:href="repoLink"
class="repo-link"
target="_blank"
rel="noopener noreferrer">
GitHub
{{ repoLabel }}
<OutboundLink/>
</a>
</nav>
Expand Down Expand Up @@ -69,14 +69,22 @@ export default {
})
}))
},
githubLink () {
repoLink () {
const { repo } = this.$site.themeConfig
if (repo) {
return /^https?:/.test(repo)
? repo
: `https://github.com/${repo}`
}
}
},
repoLabel () {
if (this.$site.themeConfig.repoLabel) return this.$site.themeConfig.repoLabel

const repoHost = this.repoLink.match(/^https?:\/\/[^/]+/)[0] || 'github'
Copy link
Member

Choose a reason for hiding this comment

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

Since repo link start with https://, so the check match(/^https?:\/\/[^/]+/)[0] is duplicated.

in other words, the statements match(/^https?:\/\/[^/]+/)[0] is dangerous since match would return undefined!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is true. Will fix this later.

return ['GitHub', 'GitLab', 'Bitbucket'].find(platform => {
return repoHost.toLowerCase().includes(platform.toLowerCase())
Copy link
Member

@ulivz ulivz Apr 20, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

In client side you shouldn't use includes and find since Buble doesn't ployfill for that.

BTW, why define a Uppercase constant and then toLowerCase?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh okay, I would change this to match instead.

Defining uppercase constants since they are what we want to show on the header, lowercase since we want to compare. But actually using regex with case insensitive flag and match would be nicer.

});
},
},
methods: {
isActive
Expand All @@ -100,12 +108,12 @@ export default {
display inline-block
margin-left 1.5rem
line-height 2rem
.github-link
.repo-link
margin-left 1.5rem

@media (max-width: $MQMobile)
.nav-links
.nav-item, .github-link
.nav-item, .repo-link
margin-left 0

@media (min-width: $MQMobile)
Expand Down