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

Format internal links as #<id> #765

Merged
merged 2 commits into from
Jan 24, 2024
Merged

Format internal links as #<id> #765

merged 2 commits into from
Jan 24, 2024

Conversation

ekzyis
Copy link
Member

@ekzyis ekzyis commented Jan 21, 2024

This formats links like https://stacker.news/items/395051 to #395051

@ekzyis ekzyis added the feature new product features that weren't there before label Jan 21, 2024
@ekzyis ekzyis force-pushed the internal-linking branch 2 times, most recently from 547ed1b to d19b0d0 Compare January 21, 2024 18:30
@huumn
Copy link
Member

huumn commented Jan 21, 2024

Excited about this one. I've been think a lot about #350 lately.

For this change, I think we'll want to handle two other cases:

  1. referral links, because many folks use the copy link action
  2. comment links using copy link which I changed here d14123c

So we'll want a regex that matches something that looks like this: https://stacker.news/items/<item id>[/r/<nym>][?commentId=<item id>]

@ekzyis ekzyis marked this pull request as draft January 21, 2024 21:53
@ekzyis
Copy link
Member Author

ekzyis commented Jan 22, 2024

With cad1258, this:

https://stacker.news/items/395051

https://stacker.news/items/395051foobar

https://stacker.news/items/395051/r/ekzyis

https://stacker.news/items/328193/r/ekzyis?commentId=395064

https://stacker.news/items/328193?commentId=395064&parentId=1241

gets formatted to:

[#395051](https://stacker.news/items/395051)

https://stacker.news/items/395051foobar

[#395051](https://stacker.news/items/395051/r/ekzyis)

[#328193](https://stacker.news/items/328193/r/ekzyis?commentId=395064)

[#328193](https://stacker.news/items/328193?commentId=395064&parentId=1241)

See https://regexr.com/7qola

Btw, I noticed that the _ in our name validator is unnecessary since underscores are included in the \w character class:

const nameValidator = string()
  .required('required')
  .matches(/^[\w_]+$/, 'only letters, numbers, and _')
  .max(32, 'too long')

@ekzyis ekzyis marked this pull request as ready for review January 22, 2024 01:43
@huumn
Copy link
Member

huumn commented Jan 24, 2024

Not a huge deal but shouldn't this [#328193](https://stacker.news/items/328193/r/ekzyis?commentId=395064) show the commentId, ie [#395064](https://stacker.news/items/328193/r/ekzyis?commentId=395064), given that's what that links to?

@ekzyis
Copy link
Member Author

ekzyis commented Jan 24, 2024

Not a huge deal but shouldn't this [#328193](https://stacker.news/items/328193/r/ekzyis?commentId=395064) show the commentId, ie [#395064](https://stacker.news/items/328193/r/ekzyis?commentId=395064), given that's what that links to?

Ah good point, I agree

@ekzyis
Copy link
Member Author

ekzyis commented Jan 24, 2024

Done in 3873cbb.

Also refactored the code. I realized we can leverage URL parsing using the URL constrcutor. No need for a complicated regexp. Also, the code now doesn't even need to care about the origin.

the power of good sleep lol

@huumn
Copy link
Member

huumn commented Jan 24, 2024

This change turns [this in my text for the url](https://stacker.news/items/328193) into [#328193](https://stacker.news/items/328193) which is probably not what you want. I think you just need to move the code block for this change to later in the function.

@ekzyis
Copy link
Member Author

ekzyis commented Jan 24, 2024

This change turns [this in my text for the url](https://stacker.news/items/328193) into [#328193](https://stacker.news/items/328193) which is probably not what you want. I think you just need to move the code block for this change to later in the function.

Good catch, fixed in c34c2a4

@huumn huumn merged commit e4aff5d into master Jan 24, 2024
1 check passed
@ekzyis ekzyis deleted the internal-linking branch February 14, 2024 00:26
@ekzyis ekzyis mentioned this pull request Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature new product features that weren't there before
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants