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

Changing repo label depending on the host #168

Merged
merged 6 commits into from Apr 20, 2018

Conversation

ycmjason
Copy link
Contributor

Fixing #167

@inosik
Copy link

inosik commented Apr 20, 2018

Maybe this should be configurable?

@ycmjason
Copy link
Contributor Author

maybe I will keep the logic now since it is quite nice to have less config. But with the config would be a fallback? would that make sense?

@inosik
Copy link

inosik commented Apr 20, 2018

I'd just keep it simple and let the user configure it with a repoLabel option. But maybe it's just me 😄

@ycmjason
Copy link
Contributor Author

I personally prefer just setting therepoLink then everything else works. Instead of having needed to set the repoLabel. Let me just quickly add this. We can wait for more opinions.

@ycmjason
Copy link
Contributor Author

@inosik added the repoLabel option. I have kept the auto generated "GitHub", "GitLab", "Bitbucket" when repoLabel is not defined.


const repoHost = this.repoLink.match(/^https?:\/\/[^/]+/)[0] || 'github'
return ['GitHub', 'GitLab', 'Bitbucket'].find(platform => {
return repoHost.toLowerCase().includes(platform.toLowerCase())
Copy link
Member

@ulivz ulivz Apr 20, 2018

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.

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.

.gitignore Outdated
@@ -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

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.

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.

Looks less stable, please test more. Thanks!

@ycmjason
Copy link
Contributor Author

@ulivz I have made some changes. Please review them again.

Here are some changes:

  • Added a guarded clause to avoid this.repoLink.match from failing since we could be confident that this.repoLink will always start with https://.
  • Used for-loop to achieve the same functionality of Array.prototype.find.
  • Used RegExp.prototype.test to check if the host of the repoLink contains any recognised platform, if not return "Source".

@yyx990803 yyx990803 merged commit c1bbd05 into vuejs:master Apr 20, 2018
@ulivz
Copy link
Member

ulivz commented Apr 21, 2018

Hmmmm.... this commit introduced a very obvious bug:

image

Fixed at f55fa00.

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.

None yet

4 participants