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

Fix issue #1905, item popover with commentId #1911

Merged
merged 7 commits into from
Feb 27, 2025

Conversation

Scroogey-SN
Copy link
Contributor

@Scroogey-SN Scroogey-SN commented Feb 16, 2025

This makes the popover show the comment instead of the post.

To test this locally, I had to modify .env.local with

PUBLIC_URL=https://localhost:3000
SELF_URL=https://localhost:3000

screenshot

Tested that

  • item links still popover as before
  • item and comment links, when clicked, still result as before

@huumn
Copy link
Member

huumn commented Feb 24, 2025

Sorry about the wait. I'm on vacation until thursday but I'll get to it first thing.

@huumn
Copy link
Member

huumn commented Feb 27, 2025

I reverted the unrelated change you made to lib/url.js - I'm not sure why that was there but if it's a fix for something else please open another pr.

I also ended up reverting the fix here - which to put it bluntly was a hack and broke other links like http://localhost:3000/items/458289/related because it was using the link text to determine the id. Instead, I made the change in the plugin that parses and passes id such that it passes the comment id if it exists.


Welcome to the team! Please send a long a lightning address when you get a chance so I can send you some sats.

@huumn huumn merged commit f97c1b0 into stackernews:master Feb 27, 2025
3 checks passed
@Scroogey-SN
Copy link
Contributor Author

Scroogey-SN commented Feb 27, 2025

Thanks! [email protected]

The lib/url.js change is for issue #1924. I should make branches for individual pull requests.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants