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

Extend comments-time-machine-links feature #1863

Merged
merged 10 commits into from Apr 20, 2019

Conversation

fregante
Copy link
Member

@fregante fregante commented Mar 18, 2019

I think that the most clear and less troublesome way to solve this is to add an icon:

  • It avoids the "What you see is not what I get issue that'd we'd have if we change links without leaving a visual cue.
  • It clearly shows that a link can be refined, rather than hiding the action behind a button (which I wouldn't know where to place anyway)

a

b

Notes

Tests

And more links in:

Possible bug, should we disable this feature in code blocks?

The clipping bug is actually due to the #1146 and also appears for linkified issues: no full solution, but moving the tooltip to the right might solve it for short links

Suggest more conversations with links like that

Closes #1448

@fregante
Copy link
Member Author

Screenshot 2019-03-19 at 01 41 32

This also updates the old links, however I kinda want to move them to the comment dropdown at some point.

@sindresorhus
Copy link
Member

This looks really useful!

@sindresorhus
Copy link
Member

All links point to the default branch instead of the linked branch because

However, this limitation considerably limits its usability. Is there any way we can do API calls to support getting the correct branch too?

@sindresorhus
Copy link
Member

/source/libs/api.js@master#L3

This test (The second under the Tests header) doesn't work.

@sindresorhus
Copy link
Member

however I kinda want to move them to the comment dropdown at some point.

👍

@sindresorhus
Copy link
Member

should we disable this feature in code blocks?

Yes

@fregante
Copy link
Member Author

fregante commented Mar 27, 2019

Is there any way we can do API calls to support getting the correct branch too?

Yes... if we must. I can test if all those /tree/name are tags. If they are, the icon isn't needed at all because tags are already semi-permalinks

This test (The second under the Tests header) doesn't work.

It does, but it was a bad test since that file didn't exist when I posted my comment, so the link is still a 404. D'oh. I changed it.

@fregante
Copy link
Member Author

However, this limitation considerably limits its usability. Is there any way we can do API calls to support getting the correct branch too?

Too involved, I tried but I'm not interested, best left as a separate issue

should we disable this feature in code blocks?

Done

however I kinda want to move them to the comment dropdown at some point.

Done

Screenshot 2019-04-18 at 15 33 22

I think this only needs docs now

@sindresorhus
Copy link
Member

I tried but I'm not interested, best left as a separate issue

Can you open an issue for it?

@fregante
Copy link
Member Author

Can you open an issue for it?

Once this is merged, as it depends on it

Copy link
Member

@sindresorhus sindresorhus left a comment

Choose a reason for hiding this comment

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

Good to merge when the docs updates are done 👌

@fregante fregante merged commit 49f57be into master Apr 20, 2019
@fregante fregante deleted the time-machine-links-for-links branch April 20, 2019 03:10
@fregante fregante changed the title Add timemachine links to all repo links Extend comments-time-machine-links feature Apr 20, 2019
@fregante fregante mentioned this pull request May 18, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

Permalinks for volatile links in comments
2 participants