-
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
fix($theme-default): fix editLink for repos hosted on gitlab.com #2523
Conversation
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 for the PR @fulopkovacs! Do you mind refactoring this to use the pattern that we're using for bitbucket
so we have clear scopes of different repositories?
d4bb396
to
c61c6da
Compare
Sure! I did not follow one detail of the BitBucket implementation though: for the Do you think that I have missed something? This part of the BitBucket-style implementation looks a bit weird to me. |
@fulopkovacs Good question. I believe it's doing that because some sites use the actual repo for the docs while others have a mono repo that contains a separate place for docs. Does that help to clarify why it's implemented that way? Definitely room for improvement, but probably should keep it consistent since I imagine it catches an edge case. |
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.
One last thing and I think we're good to go!
4fbfa2e
to
65bae3d
Compare
@@ -80,8 +80,8 @@ export default { | |||
methods: { | |||
createEditLink (repo, docsRepo, docsDir, docsBranch, path) { | |||
const bitbucket = /bitbucket.org/ | |||
if (bitbucket.test(repo)) { | |||
const base = outboundRE.test(docsRepo) ? docsRepo : repo |
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.
The second expression (repo
) of the ternary operator would never get executed in the refactored implementation, since the value of docsRepo
must be a full url (it must contain the bitbucket.org
string), which means that it will always match the outboundRE
regex pattern (which is /^[a-z]+:/i
(from line 4 of packages/@vuepress/theme-default/util/index.js
)), so the base
variable would always get its initial value from docsRepo
.
I tried to understand the original intention behind this line, but it just seemed to be wrong to me: if docsRepo
had been defined by the user as group(or user)/project
(like vuejs/vuepress
), then the base
variable used the value of repo
, which is the url of the main site's repo, and not the repo of the docs. Not to mention, that a group(or user)/project
-style docsRepo
value suggests a GitHub
repo and not a BitBucket
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.
Thanks for your work on this @fulopkovacs! The code looks much better!
…js#2523) * fix($theme-default): fix editLink for repos hosted on gitlab.com * refactor($theme-default): refactor the gitlab edit links * refactor($theme-default): refactor bitbucket edit links
Summary
Urls that open files in the web editor follow a slightly different structure on GitLab and GitHub. This is why currently the
editLink
s in VuePress are broken for every GitLab repo.The difference is that the repository's name is followed by
/-
in the GitLab links. This PR attempts to fix this issue.What kind of change does this PR introduce? (check at least one)
If changing the UI of default theme, please provide the before/after screenshot:
Does this PR introduce a breaking change? (check one)
If yes, please describe the impact and migration path for existing applications:
The PR fulfills these requirements:
fix #xxx[,#xxx]
, where "xxx" is the issue number)You have tested in the following browsers: (Providing a detailed version will be better.)
If adding a new feature, the PR's description includes:
To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.
Other information:
Tests
Unfortunately if you attempt to edit a file on GitLab without having proper permissions, you are redirected to the blob (but there are no warning messages displayed). To properly test this PR you need a GitLab account and a GitLab repo where you have rights to editing files.
Since the only file I modified (
packages/@vuepress/theme-default/components/PageEdit.vue
) has no tests and my changes are quite small and straight-forward, I did not add new test cases to this PR.Please let me know if you need more information, or if this PR doesn't meet the requirements!☺️