[openstreetmap/openstreetmap-website] Post-merge UI review for #4455 (Issue #4773)
Andy Allan
notifications at github.com
Wed May 8 16:47:41 UTC 2024
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 :shrug: )
# Globe positioning
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.
```diff
&.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
```diff
+ .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).
```diff
- <% 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
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
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
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.
--
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/issues/4773
You are receiving this because you are subscribed to this thread.
Message ID: <openstreetmap/openstreetmap-website/issues/4773 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20240508/ef4745ab/attachment-0001.htm>
More information about the rails-dev
mailing list