[openstreetmap/openstreetmap-website] Show tag changes in object history view (PR #6448)

[Tom Hughes]

@tomhughes commented on this pull request.

A couple of overall comments...

Firstly, please don't create "address review feedback" commits but instead fold the changes into the original commit(s) where the code being changed was introduced.

Secondly, maybe it's just me but to me the way this shows changes seems to be backwards to me... Consider this example:

<img width="421" height="1182" alt="Image" src="https://github.com/user-attachments/assets/c04f5b3f-ed49-4a08-ab44-56c2b4ea784d" />

That way was `leisure=park` in v1 and changed `amenity=school` to in v2 but it looks like the change is happening in v1, which is logically impossible and hence why I spotted it. Then you realise it's actually showing the change backwards as well at which point it makes a bit more sense but is assuming you're reading backwards in time from the present version.

Maybe it's just the programmer in me and normal people expect it to work like that but it's not what a programmer used to reading diffs expects to see!

> +<% key = tag[0] %>
+<% version_details = tag[1] %>
+<% rows = format_tag_row_with_change(key, version_details) %>
+<% rows.each do |row| %>
+  <%= row %>
+<% end %>

You kind of have to ask, once every line in your view is a ruby interpolation, is it serving any purpose as a view partial?

Given this is the only use of `format_tag_row_with_change` maybe that function should incorporate the rest of the logic here so there's just one interpolation?

Then once you've done that maybe the partial goes away altogether and we just call that helper in the one partial that currently invokes this partial, either iterating the collection there or changing the helper again to process the whole collection?

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6448#pullrequestreview-3872531820
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/6448/review/3872531820 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20260301/8bb4e679/attachment-0001.htm>