-
-
Notifications
You must be signed in to change notification settings - Fork 30
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
Use short posts and comments IDs in the URLs #1639
Conversation
♻️ PR Preview 4bff3e4 has been successfully destroyed since this PR has been closed. 🤖 By surge-preview |
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.
It seems the short links are not treated as internal ones and are opened in a new tab/window.
src/utils/parse-text.js
Outdated
const beforeChars = new RegExp(`[${wordAdjacentChars}]`); | ||
const afterChars = new RegExp(`[${wordAdjacentChars.clone().removeChars('/')}]`); |
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.
I think //clbn/a0f76b
should NOT be hyperlinked the same way /clbn/a0f76b/
isn't. (So I would add removeChars('/')
to beforeChars
as well.)
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.
Well, it will be linked as /[/clbn/a0f76b]
, which isn't too bad... Maybe there are some specific cases in which this can lead to incorrect results?
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.
My point is that the default behavior should be to not link text unless we are sure the user's intention is to have a link. And since I can't come up with any legitimate case for having a slash in front of a link, we shouldn't link it the same way we don't link /clbn/a0f76b/
.
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.
Ok, I'd fixed 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.
LGTM!
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.
Looks good to me!
This is the client-side part of the Short Links feature.
Added
/user/4a39b8
(a post) or/groupname/f482e5#ad2b
(a comment)./user/...
) are now parses in texts as active links.