stackernews / stacker.news

Internet communities that pay you Bitcoin
https://stacker.news
MIT License
403 stars 105 forks source link

Fix issue with popover on full sn links #1216

Closed tsmith123 closed 3 weeks ago

tsmith123 commented 3 weeks ago

Fixes #1213

Description

This PR ensures a popover is seen for internal links that include a text part.

Tests

These links were tested and are working as expected:

Link Comment
http://localhost:3000/items/459356 shows popover and renders internal link
[link](http://localhost:3000/items/458213 shows popover and renders internal link
[link](https://stacker.news) opens in new tab (this is external relative to localhost)
https://stacker.news opens in new tab (this is external relative to localhost)
[link](http://localhost:3000/settings) renders internal link (no popover)
[link](/settings) renders internal link (no popover)

Screenshots

Preview Screenshot 2024-06-03 at 07 47 26

Popover visible on second link Screenshot 2024-06-03 at 07 47 34

huumn commented 3 weeks ago

We didn't want to change the behavior of [something](/items/423) to display as #423 in this one. Otherwise, this is good. Note to self, that function is getting pretty gnarly.

tsmith123 commented 3 weeks ago

Ah, didn't consider that scenario. Yeah the function is getting messy and difficult to make changes without breaking something. Really need unit tests so when someone with little experience of the codebase makes changes they can be sure they haven't broken something.

I can start adding some unit tests around the codebase if that's any help?

huumn commented 3 weeks ago

@ekzyis and I have been discussing tests more, trying to figure out where they're good, where they're bad. We should have a public discussion at some point about it, but until then let's hold off.