[openstreetmap/openstreetmap-website] Editor color mode preference (PR #6492)

[Tom Hughes]

@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/3417754672 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20251104/19e1cfbf/attachment-0001.htm>