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

highlight comment from URL hash #3869

Closed
wants to merge 1 commit into from

Conversation

k-yle
Copy link
Contributor

@k-yle k-yle commented Jan 1, 2023

Sometimes it can be helpful to link directly to a comment on a note or changeset.

This is already possible using the URL https://osm.org/changeset/CHANGESET_ID#COMMENT_ID, but it's not clear which comment the URL is actually referring to. This PR highlights the comment from the URL hash in a red border.

image

@danieldegroot2
Copy link
Contributor

I would suggest using the existing blue highlight for the input box, like GitHub does.

OSM:
image

GitHub:
image

image

@k-yle
Copy link
Contributor Author

k-yle commented Jan 2, 2023

👍 I've changed it to match the bootstrap theme

image

@gravitystorm
Copy link
Collaborator

Thanks @k-yle for your suggestion - I think it's reasonable to highlight the comment, but I think we would need a different approach to the implementation.

We try to avoid writing our own custom CSS, and to use Bootstrap components and utilities instead. In particular this helps with future refactoring, where we can change variables (like colours, border radius, etc) in one place and for that to be applied automatically and consistently throughout the site. This means that we don't want to copy+paste values (like colour values), and we don't want to create more custom CSS to maintain.

Also, you can see there is a problem with padding within the border here - the C of Comment is touching the border, and for all other bootstrap components that have borders (e.g. alerts ) there is a gap between the border and the content. To be clear, I'm not asking you to add a padding property to your custom CSS, rather that if you want to use a border to highlight the content, then you need to pick a bootstrap component (or utility) that has a border and use that. The component will then come with appropriate padding, border radius, etc, automatically. You'll also need to use the same component, with the border turned off, for other comments so that they don't look different when highlighted (e.g. the highlighted comment is narrower due to additional padding).

Finally, there might be alternatives to having borders (and therefore additional padding), like changing the background colour or something else. I encourage you to have a think about and research what other options might fit the task of highlighting the comment that fit naturally with Bootstrap.

@gravitystorm gravitystorm marked this pull request as draft January 4, 2023 10:39
@bkil
Copy link

bkil commented May 19, 2023

I also tried to share links that jump to a direct comment of a changeset in the past, but then reconsidered.

If you share a URI with an anchor, one of the JS (forgot which one) eats it away from the logic that queries the bounding box of the changeset. Merging something like this will introduce a bug for that. It will only be a few lines of change, though.

@gravitystorm
Copy link
Collaborator

I'm going to close this PR for now. @k-yle you are welcome to open another PR addressing both the points I made above, and also investigating the point made by @bkil too.

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.

4 participants