@tomhughes commented on this pull request.
In addition to the specific comments could you rebase this please as there are
apparently some conflicts.
Having now tried this after reviewing the code I think a lot of my comments may
be explained by the fact that this only works in the history list, so I guess
`current_version` is not the current version in the normal sense but is the
version currently being rendered?
I have to say I find it a bit odd that the changes aren't shown when looking at
a specific version - it actually took me a while to find out how to trigger
this to the extent that I was putting in debugging code to try and find out why
it didn't seem to be working!
> + previous_version = all_versions
+ .find_index { |v| v.version == current_version }
+ &.then { |index| index.positive? ?
all_versions[0...index].reverse : nil }
+ &.find { |v| !v.redacted? || params[:show_redactions] }
This is trying to cope with `current_version` not existing in `all_versions`
but when would that happen? Not only would I expect it to always exist but I'd
expect it to always be at the end? In which case this should work:
```suggestion
previous_version = all_versions
.reverse
.drop(1)
.find { |v| !v.redacted? || params[:show_redactions] }
```
> +
+ def format_tag_value_with_change(key, change_info)
+ case change_info[:type]
+ when :added
+ tag.ins(format_value(key, change_info[:current], :skip_wikidata_preview
=> true))
+ when :unmodified
+ # For added and unmodified show the current value without the wikidata
preview
+ format_value(key, change_info[:current], :skip_wikidata_preview => true)
+ when :modified
+ # Return array of two values for the two rows that will be created
+ [
+ tag.del(format_value(key, change_info[:previous],
:skip_wikidata_preview => true)),
+ tag.ins(format_value(key, change_info[:current],
:skip_wikidata_preview => true))
+ ]
+ when :removed
+ tag.del(format_value(key, change_info[:previous] || "",
:skip_wikidata_preview => true))
When will a removed key not have a previous value? As far as I can see
`wrap_tags_with_version_changes` always includes it/
> + when :added
+ tag.ins(format_value(key, change_info[:current], :skip_wikidata_preview
=> true))
+ when :unmodified
+ # For added and unmodified show the current value without the wikidata
preview
+ format_value(key, change_info[:current], :skip_wikidata_preview => true)
+ when :modified
+ # Return array of two values for the two rows that will be created
+ [
+ tag.del(format_value(key, change_info[:previous],
:skip_wikidata_preview => true)),
+ tag.ins(format_value(key, change_info[:current],
:skip_wikidata_preview => true))
+ ]
+ when :removed
+ tag.del(format_value(key, change_info[:previous] || "",
:skip_wikidata_preview => true))
+ when nil
+ # For unknown versioning show the current value with the wikidata preview
+ format_value(key, change_info[:current] || "", :skip_wikidata_preview =>
false)
Same here for the current value? As far as I can see it should always have one?
> + # Generate single row for other change types
+ cells = [tag.th(format_key(key), :class => "py-1 border-secondary-subtle
table-secondary fw-normal")]
+
+ # Only add diff indicator cell if we're in a history/diff context
+ cells << tag.td(get_change_indicator_text(change_info[:type]), :class =>
get_indicator_cell_class(change_info[:type])) if change_info[:type]
+
+ cells << tag.td(format_tag_value_with_change(key, change_info), :class
=> "py-1")
Why break this down into multiple statements that append to a temporary array
when the modified version just does it all inline?
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6448#pullrequestreview-3610523986
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/6448/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
