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

[Pablo Brasero]

@pablobm commented on this pull request.

I love this ❤️ Some notes:
- The border in the middle (`border-start` of value cell) looks perhaps a bit too thin to me, particularly when contrasted with the one in the left (tag name cell). Probably not a big deal, but I wonder if other variants can be experimented.
- I see you added a background color for dark mode. To me it looks subtle enough not to clash with the text, so that's good. Still I wonder: is there a rationale for this difference?
- I wonder if there's a way to make this work with assistive technologies too. An `aria-*` tag or something, but I can't find anything right now and it's a very tricky subject anyway. Perhaps for a future PR.

> +        <% tag_changes = tag_changes_for_version(tag_details_object, @old_features) %>
+        <% tag_changes.sort.each do |key, change_info| %>

I think it's fine to have these in a single line:


```suggestion
        <% tag_changes_for_version(tag_details_object, @old_features).sort.each do |key, change_info| %>
```

On a quick search, I've seen two existing examples of what you are doing, but both look like something that should be moved to a helper instead ([app/views/layouts/_control_icons.html.erb](https://github.com/openstreetmap/openstreetmap-website/blob/6ff5918bd07673fb8db633701f77abeb16c5a1bd/app/views/layouts/_control_icons.html.erb#L1), [app/views/site/help.html.erb](https://github.com/openstreetmap/openstreetmap-website/blob/6ff5918bd07673fb8db633701f77abeb16c5a1bd/app/views/site/help.html.erb#L7-L8)).

> @@ -38,4 +38,8 @@
   <% end %>
 </div>
 
-<%= render :partial => "browse/tag_details", :object => common_details.tags %>
+<%= if defined?(@old_features) && @old_features

This shouldn't be necessary in Ruby. Instance variables (starting with `@`) resolve to `nil` if not defined.


```suggestion
<%= if @old_features
```

> @@ -0,0 +1,18 @@
+<% unless tag_details_with_changes.empty? %>
+  <h4><%= t ".tags" %></h4>
+  <div class='mb-3 border border-secondary-subtle rounded overflow-hidden'>
+    <table class='mb-0 browse-tag-list table align-middle'>
+      <% if defined?(@old_features) && @old_features %>

Same as above:

```suggestion
      <% if @old_features %>
```

On app/views/browse/_tag_with_changes.html.erb:

There's a lot of duplication across `_tag`/`_tag_with_changes` and `_tag_details`/`_tag_details_with_changes`. I wonder if they can be merged into only `_tag` and `_tag_details`, with conditionals to show the changes only when available. In this case you will need `defined?` as `tag_details_object` is a local variable, as opposed to an instance variable that will be nil if undefined.

> +    changes = {}
+
+    # Check for added and modified tags
+    current_tags.each do |key, value|
+      if !previous_tags.key?(key)
+        changes[key] = { type: :added, current: value }
+      elsif previous_tags[key] != value
+        changes[key] = { type: :modified, current: value, previous: previous_tags[key] }
+      else
+        changes[key] = { type: :unchanged, current: value }
+      end
+    end

I try to avoid `each` for these things because it often means "anything could happen here!". Using idiomatic methods like `each_with_object` helps communicating that, eg: "we are going to build a hash step by step":

```suggestion
    changes = current_tags.each_with_object({}) do |(key, value), memo|
      if !previous_tags.key?(key)
        memo[key] = { type: :added, current: value }
      elsif previous_tags[key] != value
        memo[key] = { type: :modified, current: value, previous: previous_tags[key] }
      else
        memo[key] = { type: :unchanged, current: value }
      end
    end
```

> +    previous_tags.each do |key, value|
+      unless current_tags.key?(key)
+        changes[key] = { type: :deleted, previous: value }
+      end
+    end

Another way to do this. In Ruby you can do Array difference with `-`. Also you are not using the `previous` for deleted tags, so it can be removed:

```suggestion
    (previous_tags.keys - current_tags.keys).each do |key|
      changes[key] = { type: :deleted }
    end
```

I have to admit that in this instance I'm not 100% sure if my alternative is too clever. Thoughts?

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

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