Skip to content

add a tag diff to /(node|way|relation)/:id/history#6995

Open
stillhart wants to merge 1 commit into
openstreetmap:masterfrom
stillhart:historydiff
Open

add a tag diff to /(node|way|relation)/:id/history#6995
stillhart wants to merge 1 commit into
openstreetmap:masterfrom
stillhart:historydiff

Conversation

@stillhart
Copy link
Copy Markdown
Contributor

Description

I added a tag diff between change sets to highlight the changes of tags.

The diff is not expanded by default. I wanted the diff in between the change sets to avoid confusion with the given state at that time. I wanted the diff to be opt-in.
I used the default bootstrap colors which contrasts many other implementations.
I didn't want any strike through text.

Relates to #738 #1253 #4765

Screenshots

Expanded

image

Without expansion

image

How has this been tested?

  • I ran the (new) tests
    • docker compose exec web bundle exec rails test test/system/element_history_test.rb test/helpers/browse_tags_helper_test.rb
  • I tested it manually on my docker instance

What should be discussed?

  1. It won't show a diff across pagination since I didn't want to mess with record loading. This is by design at the moment. Could be added by just giving the record to the partial.
  2. Design is ok but somewhat lacking, but is using the existing bootstrap design.
  3. I tried to be screen reader friendly since colors are not obvious, but I'm not certain about that.
  4. I don't like the text Tag changes from version #1 to #2 all that much.
  5. Additions green, removals red, but modifications could also be two shades of blue. Or two other shades of green and red.
  6. Naming of the two change sets might need improvement inside the code:
    • old new
    • previous next
    • before after

None of the above seem to be deal breakers for me.

Note: I was AI assisted.

@stillhart
Copy link
Copy Markdown
Contributor Author

Just came across #6448. That one has kinda nicer styling but I don't like the "in change set view". Mine seems to be a different approach. An obvious diff that is explicitly in between the two affected records.

@tomhughes tomhughes added the ai-assisted Suggestions making extensive use of outsourced intelligence label Apr 11, 2026
@tomhughes
Copy link
Copy Markdown
Member

tomhughes commented Apr 11, 2026

As you have apparently now discovered there is already a well developed PR working on this that has had a number of rounds of review so I think it's likely that is the best place to be working on things.

@1ec5
Copy link
Copy Markdown
Collaborator

1ec5 commented Apr 12, 2026

An obvious diff that is explicitly in between the two affected records.

Maybe that could go on a separate page that’s dedicated to diffing two versions of an element, as in #6761? That could be in addition to what’s going on in #6448.

<% if old_element %>
<% diffs = tag_diff(new_element.tags, old_element.tags) %>
<% if diffs.any? %>
<details class="mb-3 border-bottom border-secondary-subtle pb-3">
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

A possible downside of the collapsible design is that the find in page function in some browsers1 won’t be able to find the contents until you manually go through opening each section. I have a hunch that people use this page as a sort of blame view. After all, that’s the main advantage of keeping everything on this one history page instead of relegating the diff to the version page or a separate diff page.

Footnotes

  1. For example, Firefox only started finding in closed <details> elements in 139.0.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am the sort of person who is continually annoyed by not being able to find text in a page for reasons such as this one 😬

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-assisted Suggestions making extensive use of outsourced intelligence

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants