[openstreetmap/openstreetmap-website] Facelift `offline.html` and use Bootstrap classes for "notifications" under the search bar (PR #3907)

[Andy Allan]

@gravitystorm requested changes on this pull request.

Thank you for the PR! I've made some requests as inline comments, and I'm happy to discuss them further if required.

> @@ -1,5 +1,5 @@
 <% if flash[:error] %>
-  <div class="flash error row mx-0 p-3 align-items-center">
+  <div class="alert alert-danger row mx-0 mb-0 p-3 rounded-0 align-items-center">

I'm happy to see the move from custom CSS to use the built-in bootstrap alert panels and colours. There's a couple of things here that need addressing though:

* If the `flash` CSS is no longer required elsewhere, then it should be removed from `common.scss`
* Large numbers of tests are failing, since they are looking for the `flash` and `error` css classes. The tests need to be updated.

>      <p><%= t "layouts.osm_offline" %></p>
+    </div>

* The contents of this `div` need to be indented. The same with other places where you are wrapping existing code with additional `divs`.


>    <% elsif Settings.status == "database_readonly" or Settings.status == "api_readonly" %>
+    <div class="alert alert-danger text-center">

Offline and read_only modes are not "danger" messages, since there's nothing that the user can or has done wrong. 

If we look at the `map_layout` helper in https://github.com/openstreetmap/openstreetmap-website/blob/e443d99edd9c9b68d6c0fa41b028bb9a5cdb07c5/app/controllers/application_controller.rb#L258-L263 we can see that both are treated as `warning` messages

> @@ -1,7 +1,9 @@
+<div class="alert alert-danger text-center">

For both the offline and edit pages, we end up with "alert"-style banners, but these don't match the flash banners used on the rest of the site, namely:

* They don't stretch across the whole page
* They don't have the icon alongside the message

It would be nice to make them completely consistent, rather than partially consistent. So for example, I would consider reworking this page to use the existing flash messages system to render the alert, for example by using `flash.now[:warning] = t("layouts.osm_offline")` in the controller. That way any changes made to flash messages elsewhere (e.g. adjusting margins or paddings) will apply automatically to these offline alert messages.

-- 
Reply to this email directly or view it on GitHub:
https://github.com/openstreetmap/openstreetmap-website/pull/3907#pullrequestreview-1269137006
You are receiving this because you are subscribed to this thread.

Message ID: <openstreetmap/openstreetmap-website/pull/3907/review/1269137006 at github.com>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.openstreetmap.org/pipermail/rails-dev/attachments/20230125/93064b3f/attachment-0001.htm>