@tomhughes commented on this pull request.
> @@ -3,7 +3,7 @@
<%= javascript_include_tag "turbo", :type => "module" %>
<%= javascript_include_tag "application" %>
<%= javascript_include_tag "i18n/#{I18n.locale}" %>
- <% if preferred_color_scheme(:site) == "auto" %>
+ <% if preferred_color_scheme(:site).nil? %>
If you reversed the branches here then you wouldn't need the `.nil?` which
might be a bit clearer?
> :class => "form-select" %>
</div>
<div class="mb-3">
<%= label_tag "map_color_scheme", t(".preferred_map_color_scheme"), :class
=> "form-label" %>
<%= select_tag "map_color_scheme",
options_for_select(%w[auto light dark].map { |scheme|
[t(".map_color_schemes.#{scheme}"), scheme] },
- preferred_color_scheme(:map)),
+ current_user.preferences.find_by(:k =>
"map.color_scheme")&.v || "auto"),
This seems like a step backwards but see my suggestion for changes to
`preferred_color_scheme` for how we might avoid it.
> @@ -275,11 +275,12 @@ def preferred_editor
end
def preferred_color_scheme(subject)
- if current_user
- current_user.preferences.find_by(:k => "#{subject}.color_scheme")&.v ||
"auto"
- else
- "auto"
- end
+ scheme = current_user ? current_user.preferences.find_by(:k =>
"#{subject}.color_scheme")&.v : nil
Can this not use safe navigation like this?
```suggestion
scheme = current_user&.preferences.find_by(:k =>
"#{subject}.color_scheme")&.v
```
> @@ -275,11 +275,12 @@ def preferred_editor
end
def preferred_color_scheme(subject)
I think that rather than taking a single subject here and then having to
special case `:site` when deciding whether to fallback this could take
`*subjects` and then try each one in turn until it finds one that is set to
something other that `auto`.
Then you could do a call like `preferred_color_scheme(:map, :site)` when you
want fallback or `preferred_color_scheme(:map)` when you don't?
Possibly it could also take a default value to return as a last resort instead
of `nil` but that would probably need a keyword argument so it might be easier
to stick with `||` at the call site unless we just go back to returning `auto`
in that case.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/6492#pullrequestreview-3417754672
You are receiving this because you are subscribed to this thread.
Message ID:
<openstreetmap/openstreetmap-website/pull/6492/review/[email protected]>_______________________________________________
rails-dev mailing list
[email protected]
https://lists.openstreetmap.org/listinfo/rails-dev
