backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 40 forks source link

[UX] Maintenance mode: Do not render the "Create new account" tab, the navigation menu and the account menu when in the "Log in" or "Reset password" pages. #2529

Open klonos opened 7 years ago

klonos commented 7 years ago

This is how the "Login" and "Reset password" pages are rendered when in maintenance mode (for non-authenticated users):

backdrop-maintenance_mode-login_page

What's wrong here:


PR by @klonos: https://github.com/backdrop/backdrop/pull/1789

klonos commented 7 years ago

...here's the password reset page when in maintenance mode for non-authenticated users:

backdrop-maintenance_mode-password_reset_page

Same issues as the login page. Additionally:

klonos commented 7 years ago

PR that addresses only showing/hiding the "Create new account" tab up for review.

I initially tried using menu_cache_clear_all();, then added cache('page')->flush(); and then replaced these two with cache_clear_all();, but that did not work. Each time that the site was switched into/out of maintenance mode, the "Create new account" tab would either remain hidden when it was supposed to be shown or the other way around. Using 🏠 -> Flush all caches from the admin bar would make the tab show/hide properly though(???).

What finally worked was using menu_rebuild(); which I take might be a bit too "drastic", but as explained, nothing else seemed to work. Suggestions/ideas welcome.

klonos commented 7 years ago

...PR updated to not render the breadcrumbs or the header menu when in maintenance mode unless the user has the "Use the site in maintenance mode" permission (which renders them in the admin theme + the frontend theme when the user has the permission). When the site is in maintenance mode, this effectively hides the header menu and the breadcrumbs from the "Log in" and "Reset password" pages.

klonos commented 7 years ago

OK, updated the PR to also not be rendering any menus when in maintenance mode. Here's how the login/reset page looks when in maintenance mode for all core themes:

backdrop-issue2529-pr1789-basis backdrop-issue2529-pr1789-bartik backdrop-issue2529-pr1789-seven

quicksketch commented 7 years ago

I gave the PR a review and it could use some tweaks.

But I have questions about this suggestion. It doesn't seem right that we would be adjusting the front-end display based on the maintenance mode status. Hiding certain UI elements could have all sorts of unexpected impact on the theming and UX. While true that accessing any page besides /user/login is likely to take you to the maintenance page, I think the login page should stay the same from a design-perspective (or defer such decisions to the front-end theme). If you had a "fat footer" for example, where 1 of the 3 columns was a menu but the other two were a different kind of element (e.g. a contact block, mailing list, or something else), it would be exceeding weird to have the menu disappear from the footer while the other elements remained.

The same goes for the breadcrumb, we don't know how the breadcrumb will be implemented into the design, so we can't just hide it based on maintenance page status.

quicksketch commented 7 years ago

Moving this to 1.6.2, but I'm questioning whether this should be considered a bug.

klonos commented 7 years ago

It is more of a usability issue: elements that behave like "broken" when in maintenance mode, should not be made available to users in the first place. What's the point of rendering any menu when all links point to a "site under maintenance page"? Why not render them for the rest of the website then?

The only two pages/paths that currently "work" during maintenance mode are the login and password reset pages (so that site admins do not get locked out). As we have it now, when accessing these, the user gets some stripped-down version of the theme where certain things "do not work". It is these things that I am proposing to remove.

I see no point in letting this up to each theme for three reasons: 1) we'd have to "fix" all core themes which seems more work than handling this "centrally", 2) people might customize their core theme/layouts to include more ("broken" when in maintenance mode) menus in the header, and 3) we'd leave contrib themes in the old "broken" state unless their maintainers are aware of this issue and have the time/energy to do something in order to "fix" their themes. If we fix this in core, their themes benefit as well with 0 effort on their end.

klonos commented 7 years ago

...What's the point of rendering any menu when all links point to a "site under maintenance page"?...

There is the case of menu links pointing to external sites that will still work of course, but it makes more sense to be placing these in the "Message to display when in maintenance mode" text field or in a custom block placed in the footer I guess.

The other idea I had was to offer a "Site status is" visibility condition with values "online"/"in maintenance mode" for all blocks and have menu blocks default to hidden when in maintenance. The reason why I didn't mention this before is that although it would offer more flexibility for those that still want to render "broken" menus when in maintenance, would these be the 80% cases? I don't think so.

klonos commented 7 years ago

The same goes for the breadcrumb, we don't know how the breadcrumb will be implemented into the design, so we can't just hide it based on maintenance page status.

The breadcrumb falls into the menu category because it is comprised by things that are links to pages ...that will be "broken" when in maintenance mode. So why render it in the first place.

I have taken care in my PR to not to hide the breadcrumb when:

  1. the site is online
  2. the site is in maintenance mode, but the user is logged in (implies they have the "Use the site in maintenance mode" permission), which in turn makes it so that the breadcrumb is still visible when:
    • the user uses the admin UI (implies site admin which already has "Use the site in maintenance mode" permission by default)
    • the user uses the front-end theme (implies a user that was granted the "Use the site in maintenance mode" permission by the admin)

So, I believe I have covered pretty much all cases, making sure that the breadcrumb will only be hidden when the site is in maintenance mode and the user is either non-authenticated (anonymous site visitor) or does not have the "Use the site in maintenance mode" permission.

docwilmot commented 7 years ago

I actually agree with @klonos that it seems a bit incorrect to render all these things when we know they are inaccessible. But I also see with @quicksketch about theming concerns.

I propose that we have the login form use the maintenance page template when site is offline, since that avoids both concerns. Hows that?

klonos commented 7 years ago

Sounds reasonable @docwilmot but leaving it up to the template/theme, means that...

I see no point in letting this up to each theme for three reasons: 1) we'd have to "fix" all core themes which seems more work than handling this "centrally", 2) people might customize their core theme/layouts to include more ("broken" when in maintenance mode) menus in the header, and 3) we'd leave contrib/custom themes in the old "broken" state unless their maintainers are aware of this issue and have the time/energy to do something in order to "fix" their themes. If we fix this in core, their themes benefit as well with 0 effort on their end.

klonos commented 7 years ago

...unless you are suggesting fixing things only in Basis and ignoring the issue elsewhere that is.

docwilmot commented 7 years ago

Because the maintenance page template already excludes everything useless on the page, and every theme will already have themed it. There is a core maintenance.tpl.php so every theme will inherit from that. Get the the login form wrapped in that template and we're done.

klonos commented 7 years ago

There is a core maintenance.tpl.php...

Ahh! ...was not aware. That totally makes sense then :wink:

klonos commented 7 years ago

Get the the login form wrapped in that template and we're done.

How would we be removing the "Create new account" tab then?

jenlampton commented 7 years ago

How would we be removing the "Create new account" tab then?

hook_menu_alter() should do the trick.

klonos commented 7 years ago

Sorry for taking long to respond in issues and PR reviews/suggestions. As I said, I'll start being a bit more available from next week. Might take a look at this now though.