[openstreetmap/openstreetmap-website] Separate forms for preferences and profile (#3257)

[Tom Hughes]

@tomhughes commented on this pull request.



> +  layout "site"
+
+  before_action :authorize_web
+  before_action :set_locale
+
+  authorize_resource :class => false
+
+  before_action :check_database_readable
+  before_action :check_database_writable, :only => [:update]
+
+  def show; end
+
+  def edit; end
+
+  def update
+    current_user.languages = params[:user][:languages].split(",")

I think this needs a `set_locale` call to ensure that any changes are active? You removed it from the user settings but don't seem to have added it back here...

> @@ -0,0 +1,12 @@
+<% content_for :heading do %>
+  <h1><%= t ".title" %></h1>
+<% end %>
+
+<%= bootstrap_form_for current_user, :url => { :action => :update } do |f| %>
+  <%= f.select :preferred_editor, [[t("editor.default", :name => t("editor.#{Settings.default_editor}.name")), "default"]] + Editors::AVAILABLE_EDITORS.collect { |e| [t("editor.#{e}.description"), e] } %>
+
+  <%= f.text_field :languages %>
+
+  <%= f.primary t(".save") %>
+  <%= link_to t(".cancel"), preferences_path, :class => "btn btn-link" %>

Would `btn-outline-secondary` be a better a choice here? Having a link alongside a button looks a little odd to me?

> +    <div id="homerow" <% unless current_user.home_lat and current_user.home_lon %> class="nohome"<% end %>>
+      <p class="message text-muted"><%= t ".no home location" %></p>
+      <div class="form-row">
+        <%= f.text_field :home_lat, :wrapper_class => "col-sm-4", :id => "home_lat" %>
+        <%= f.text_field :home_lon, :wrapper_class => "col-sm-4", :id => "home_lon" %>
+      </div>
+    </div>
+    <div class="form-check">
+      <input class="form-check-input" type="checkbox" name="updatehome" value="1" <% unless current_user.home_lat and current_user.home_lon %> checked="checked" <% end %> id="updatehome" />
+      <label class="form-check-label" for="updatehome"><%= t ".update home location on click" %></label>
+    </div>
+    <%= tag.div "", :id => "map", :class => "content_map set_location" %>
+  </fieldset>
+
+  <%= f.primary t(".save") %>
+    <%= link_to t(".cancel"), user_path(current_user), :class => "btn btn-link" %>

Same comment here about the class for the cancel button/link.

> @@ -165,6 +165,12 @@
 
     <div class="user-description richtext text-break"><%= @user.description.to_html %></div>
 
+    <% if current_user and @user.id == current_user.id %>
+      <div class="my-3">
+        <%= link_to t(".edit_profile"), edit_profile_path, :class => "btn btn-outline-primary" %>

I'm not sure about the placement of this - it looks a bit odd to me. That may be partly because it's above the extra user data line that administrators like me get but I think it would stick out a bit without that.

Could we maybe put it on the right at the top? or would that be an issue on small screens?

-- 
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3257#pullrequestreview-709048087
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20210718/d3721bb2/attachment.htm>