Post-merge UI review for #4455

[gravitystorm]

This issue is a post-merge UI review for #4455 . cc @milan-cvetkovic

(I haven't done a post-merge review before, so I'm not sure if there's a better way to do it than to create a new issue - adding a review to a closed PR doesn't seem right either 🤷 )

Globe positioning

(in progress see #4793)

The globe has been repositioned using an arbitrary offset. This appears to be to keep it clear of the tabs, but it doesn't take into account translations.

   &.new-user-main {
     background-image: image-url("sign-up-illustration.png");
+    background-position-x: 50px;
   }

The offset should be removed, or designed to adapted to different sized tab labels.

Page width

+ .auth-container {
+  max-width: 600px;
+ }

This is a random width, not based on anything from the bootstrap grid. It makes the login and signup pages inconsistent with the rest of the site.

Tab alignment

The edge of the tabs should be aligned with the edge of the content. See the traces page for a working example.

Top menu buttons

The login/signup buttons disappear from the top menu in certain circumstances, and it leads to jarring inconsistencies. It means the rest of the menu moves around, and it also still doesn't work as intended (e.g. if you make a mistake while signing up, the signup page is shown with errors and the buttons reappear).

- <% else %>
+ <% elsif (controller_name != "users" and controller_name != "sessions") || action_name != "new" %>

This should be reverted. If there's still a desire to remove these buttons (e.g. from the narrow screen layout) then let's discuss that separately.

auth button sizes

The auth button sizes were reduced from 36px to 24px, but I didn't see any explanation why making them smaller is better?

sizing for auth_button vs auth_button_preferred ✔️

Fixed in b4f719a

In app/helpers/user_helper we no have two different ways to describe the size of the icon, these should be consistent:

                :size => "24"),

                :width => "24px",
                :height => "24px")

bg-white ✔️

Fixed in #4781

The "Log into OpenstreetMap to access..." panel uses bg-white, but text on a forced-white background doesn't work in dark mode

auth_button_preferred css classes

:class => "auth_button fs-6 border rounded text-muted text-decoration-none py-2 px-4 d-flex justify-content-center align-items-center",

This list of classes is trying to style the link as a button, but either an actual button or at least a btn class should be used instead. This will ensure consistency with other buttons on forms etc. .btn.btn-outline-secondary could be a good place to start.

auth_button_preferred alignment ✔️

Fixed in #4849

The alignment here isn't great. I think it might look better if the row of buttons was divided in the exact centre, with the preferred button then centered on the left half, and the remaining buttons centered on the right half. The current situation does have two divs displayed side-by-side, but with asymetric margins (mx-2 on one, and nothing on the other).

They look good on narrow screens.

If the design intention is just to have one set of buttons centred, then the gap between the preferred button and the remaining buttons is too large.

"or" divider

The "or" divider looks cramped, too close to the form labels below, and could probably do with a standard mb-3 to space things out.

"hr with words"

The PR introduced a new UI convention of using a horizontal line with words in the middle (e.g. "or" or "or sign up with a third party"). This has been implemented by using borders on empty divs, and with the same verbose code copy+pasted a few times:

    <div class="d-flex justify-content-center align-items-center">
      <div class="border-bottom border-1 flex-grow-1"></div>
      <div class="text-secondary mx-3"><%= t ".or" %></div>
      <div class="border-bottom border-1 flex-grow-1"></div>
    </div>

If there are suggestions for a better way to implement this, that would be great. It's effectively a horizontal rule, or a section divider, and there might be a more semantic way to implement this, that we can reuse in other forms and in other places around the site. In particular, I don't think this implementation is accessible, since there's no meaningful markup surrounding what is then an isolated word.

Translations broken for password field label ✔️

See #4860 for the fix, c4347c8 was the commit

autocomplete=off for password form on login ✔️

See #4860 for the fix, c4347c8 was the commit

Raw html input used for password field

c4347c8 refactored the login form to move the help text around, but in the process moved away from using form helpers and introduced raw html to define the input field. This should be refactored to use a form helper, like f.password_input_tag or similar, so that the html attributes are generated automatically, and any site-wide overrides we have on form helpers will also apply to this field.

Reuse of link label is hard to translate

Signup form now reuses privacy_policy_link and respective link label in two different sentences (in email help html and by signing up html messages). Preferably link labels in different sentences should be separate translatable messages as different declension may be needed.

[AntonKhorev]

The globe has been repositioned using an arbitrary offset

The globe images have an arbitrary padding baked in.

[AntonKhorev]

I'm not sure if there's a better way to do it than to create a new issue

Create multiple issues that can be closed separately when things are fixed?

[Pikse]

I'm not sure if this is in scope of this issue but I've two i18n remarks associated to #4455:

• Login form no longer picks up existing translations for word "Password", e.g. see https://www.openstreetmap.org/login?locale=de.
• Signup form now reuses privacy_policy_link and respective link label in two different sentences (in email help html and by signing up html messages). Preferably link labels in different sentences should be separate translatable messages as different declension may be needed.

[gravitystorm]

I'm not sure if this is in scope of this issue but I've two i18n remarks

Thanks @Pikse for you comments! You are right that both of these need fixing, and while I was looking more closely at that code I found another couple of issues too. I've added all 4 to the original comment above.

If you spot any more problems, please let us know.

[Pikse]

One more remark on message reuse: it appears that users.new.tou message was added to be used as a link label in users.new.by signing up_html sentence but actually it reuses previous link label layouts.tou which is also associated to users.terms.tou_explain_html sentence.

[talllguy]

Would addressing

Raw html input used for password field

address #4543 ?

Using a field designed for passwords may automatically add the show password element.

[HolgerJeromin]

Using a field designed for passwords may automatically add the show password element.

Not in every browser. IIRC edge does.

[talllguy]

edge does.

Yes. Looks like that wouldn't be a universal solution then.

[gravitystorm]

Would addressing

Raw html input used for password field

address #4543 ?

No, those are unrelated. This item is about what is written in the erb template, not what is outputted in the final rendered html. Specifically, the mentioned commit did something along the lines of:

-  <%= f.password_field :password, :label => t(".password"), :tabindex => 2, :value => "", .....
+  <input class="form-control mb-3" type="password" name="password" id="password" .....

i.e. swapped from using a rails form builder, and instead used raw html. Both approaches still output the same html <input type="password" ...> field.