[UX] Add a "Back to site" button into the Admin Bar.

klonos commented 7 years ago

Describe your issue or idea

This is basically a request to have https://www.drupal.org/project/escape_admin in core instead of (or in addition to) the home button. From that project's description:

This is a Drupal 7 backport of the functionality added to the revamped toolbar: ~#787896: Add a link so that administrators can return to their most recently visited non-admin page~ which adds a 'Back to site' button when the user has clicked on an administration URL from a non-administration page. The 'Back to site' link will persist until the user returns to a non-administration page.

Steps to reproduce

The UX- with the current situation is that we are assuming that the user wants to return to the home page after dealing with any admin/backend task, whereas the most likely thing is that they want to return back to the page where they started from.

For example: as a novice admin, go to any page of your site -> realize that you want comments enabled -> edit the page -> realize that comments are not enabled for the page content type -> go to Structure -> Content types -> Page -> make the changes -> Save. Now, if you click the Home button, you are taken to the home page, while you most likely needed to navigate back to the page you started from.

Actual behavior

The "Home" button takes the user to their home page. This is fine, but they already have a "home" trail in the admin theme that they can use if they want to go to the home page.

Expected behavior

A much better UX would be to have it so that when the user navigates to any admin page from the frontend, the path of the page they started from is stored in their browsing session. This path persists as they navigate through the admin interface, until they hit the "Back to site" button. What happens then is that they are taken to the page where they started from. As the user navigates through the frontend, their session is updated to store the last non-admin page they visit before switching to the admin UI.

If we think that changing the current behavior of the "Home" button would be disrupting the way current users are used for things to work, then we can either make this an option in admin/config/administration/admin-bar or add a separate "Back to site" button.

PR by @alexfinnarn : ~https://github.com/backdrop/backdrop/pull/2040~ PR by @keiserjb: https://github.com/backdrop/backdrop/pull/4236

Minor release update:

[x] Add a conclusion suitable for blog post + release notes
[x] N/A Add Documentation label if the feature needs to be documented (or needs docs to be updated)
[x] N/A Add Needs change record label if the the issue contains an API change.
[x] Add Needs translation label if the the issue contains string translations.

conclusion suitable for blog post

The home icon in the admin bar has been replaced by a Back to site link that will return people to whichever page they were on before navigating into the editorial interfaces of the site.

alexfinnarn commented 6 years ago

Here is a start...

https://github.com/backdrop/backdrop/pull/2040

There are race conditions and the icon CSS change flicker to work out, but it does what is described above using JS from the Escape Admin Drupal 7module.

olafgrabienski commented 6 years ago

I like the idea of a "Back to site" button. (In the PR it's not button but a link so far.)

@alexfinnarn I had a look at the sandbox site and noted some strange things:

As you said, there is a flickering moment (while the "Home" button gets replaced by the "Back to site" link). I'd prefer anyway not to exchange "Home" by "Back to site" but to add a separate "Back to site" button to "Home". If we did so, would that solve the flickering issue?
The "Back to site" link worked even before I enabled it on admin/config/administration/admin-bar. (It continued to work after I enabled it.)
On some admin pages, the "Back to site" link didn't appear at all (or: changed back to the "Home" button). Seemed quite arbitrary to me: no matter if the "Back to site" link was enabled or not, there were only some but different admin pages where the link didn't work as expected.

alexfinnarn commented 6 years ago

Thanks for testing out the PR and giving those detailed notes @olafgrabienski!

I now see that I wasn't using the way that the other JS behaviors attach themselves to the main admin bar function. Now that I've made that change I don't see any filcker. The changes I couldn't seem to catch end up happening when the admin bar is hidden to users so that is why the flicker cleared up after I inserted the escape admin funciton into the loop of called behaviors.

I tried to add a test to adminbar.test, but it doesn't work currently. I'm not sure how to fix that at this point.

The "Back to site" link worked even before I enabled it on admin/config/administration/admin-bar. (It continued to work after I enabled it.)

I added a default config option for back_to_site so it should now be disabled on install.

I haven't seen any of the other strange things you've pointed out since I updated the PR. Hopefully, I can figure out how add a test for this or someone can help me out with that.

I don't care if the "Back To Site" link is an icon or just the text so if people want to make it an icon, then I can add the image. However, I'm no sure there is a good icon for "Back To Site" that would be universally understood.

screen shot 2017-11-20 at 12 40 09 pm

The Drupal module seems to always have a <- back arrow and just change the text from "Home" to "Back to site".

olafgrabienski commented 6 years ago

Now that I've made that change I don't see any flicker.

Same here.

I added a default config option for back_to_site so it should now be disabled on install.

Didn't test that on a fresh installation. (At the sandbox site, the "Back to Site" option was enabled, probably because someone had enabled it before.)

I haven't seen any of the other strange things (...)

On one admin page (Add content), the Home link first wasn't replaced. I cleared all caches then, and everything was fine.

I'm no sure there is a good icon for "Back To Site" that would be universally understood. The Drupal module seems to always have a <- back arrow (...)

Back arrow + text is fine in my opinion.

Finally, one other observation: seems that the "Back to Site" link's destination is the system path (e.g. node/1) even if the last visited frontend page has an URL alias. Is it possible to use the URL alias as destination, if one exists? (Unfortunately, Backdrop doesn't redirect from system path to alias yet, see #2871; would probably be better to get that issue fixed than to work around it here and at other places.)

olafgrabienski commented 6 years ago

@klonos Are you maybe up to have a look at the PR for this feature request?

ghost commented 6 years ago

Assuming the sandbox site shows the latest commits, then here's some things I noticed while playing with this (ignore anything that's already been fixed if the sandbox is old):

The 'Back to site' link doesn't line up with the other menu items (see screenshot below)
Only the first letter 'B' should be uppercase, the rest should be lowercase, same as other menu items (e.g. 'Back to site')
Hovering over 'Back to site' shows a menu with options to flush caches, run cron, etc. This seems weird IMO and maybe we should consider moving these sub-menu items elsewhere...?
The setting for enabling/disabling this link says 'Add Back To Site Link' - the 'L' in 'Link' should be lowercase (e.g. 'Add Back To Site link')
The description for that option could use some re-wording; maybe something like "Changes the Home link/icon to a 'Back to site' link that links to the last non-admin page you visited."
Enabling/disabling the link on the 'Administration bar' page has no effect on the link, nor does refreshing/clearing caches. I had to go back to a non-admin page, click on another non-admin page, then go back to an admin page to see the new link. The 'Enabled components' at least appear/disappear when saving the form after toggling them.
No icon shown...? (this is what makes me think the sandbox site isn't updated with the latest commits...)

Other thoughts:

Maybe the 'Back to site' checkbox setting can be moved to the 'Enabled components' section instead, in addition to the Home/Icon menu option (i.e. allow both to be shown at once, either one, or none) - this would address my third point above, and also it's confusing to tick the box for the 'Back to site' link, but not have it appear because the 'Icon menu' is disabled (i.e. there's no indication the two are related or that one relies on the other).

back-to-site

opi commented 6 years ago

reviewed here https://github.com/backdrop/backdrop/pull/2040#pullrequestreview-121652429

Hovering over 'Back to site' shows a menu with options to flush caches, run cron, etc. This seems weird IMO and maybe we should consider moving these sub-menu items elsewhere...?

this is weird, but probably out of scope, as this sub items are already there, below the Home link.

docwilmot commented 6 years ago

When I view the sandbox I just get the normal Home icon. Am I missing something?

mikemccaffrey commented 6 years ago

Hovering over 'Back to site' shows a menu with options to flush caches, run cron, etc. This seems weird IMO and maybe we should consider moving these sub-menu items elsewhere...?

I created https://github.com/backdrop/backdrop-issues/issues/1368 a while back about moving the admin tasks out from under the home icon, which would solve this issue as well.

Design-wise, I think the back to site button should have a left arrow to set it apart from the regular menu items.

klonos commented 6 years ago

When I view the sandbox I just get the normal Home icon. Am I missing something?

Yeah, this is not working as supposed to...

@docwilmot if you click the "edit" tab while on the "About" page, or any content page really, then you will see the "Back to site" link. This is the bit that works as expected 👍🏼

When you click any other option in the admin menu while on the front page, the "Back to site" link does not appear. This is the bit that does not work.

STR:

log into the PR sandbox -> you are taken to /user/1
click on the "Edit" link -> "Back to site" link appears 👍🏼
click on any link on the admin menu -> "Back to site" link appears 👍🏼
click on "Home" in the main menu (or any way to get you to the front page of the site)
click on any link on the admin menu -> "Back to site" link does NOT appear 👎🏼
click on the 🏠icon in the admin bar (or the "Home" breadcrumb link)
click on the "About" link in the main menu
click on the "Edit" link -> "Back to site" link appears 👍🏼
click on any link from the admin bar -> "Back to site" link appears 👍🏼

I hope this helps.

keiserjb commented 2 years ago

Catching up on this issue because the lack of the Back to Site link has become an annoyance lately.

klonos commented 2 years ago

The PR for this is now many years old, so it will need a rebase + there was some feedback from @opi that seems that was never addressed either.

@alexfinnarn are you hovering around the Backdrop community enough to pick this up, or is it OK for someone else to work on this?

alexfinnarn commented 2 years ago

Nice to see this hopefully going into Backdrop, but feel free to do as you please with any code I have in a PR. It's probably been five years since I've looked into this issue so my knowledge is nowhere close to hovering mode.

keiserjb commented 2 years ago

The PR didn't work for me but that's alright. Hopefully I will dig into this sometime soon and get it working. I spent the last few weeks primarily working in D. Upon returning to B this feature was greatly missed.

keiserjb commented 2 years ago

I added the patch again to my local and got it to work. If I comment the

//&& settings.admin_bar.frontpage !== escapeAdminPath

line out in the javascript everything appears to work.

keiserjb commented 2 years ago

I've got it working with the icon changing.

argiepiano commented 2 years ago

@keiserjb, can you submit a new PR with those changes?

klonos commented 2 years ago

Yes please! ...very keen to review/test this feature.

keiserjb commented 2 years ago

PR, did I do it right? https://github.com/backdrop/backdrop/pull/4236

argiepiano commented 2 years ago

@keiserjb, the PR went through correctly! Someone needs to approve the automated test workflow (it looks like your github account doesn't have the needed privileges to trigger those).

I'll take a look at the PR later. Thanks again!

argiepiano commented 2 years ago

@keiserjb I applied the patch manually in my installation, but the setting "Add Back To Site Link" found at admin/config/administration/admin-bar is not saving when I turn it on and click Save. Same problem when testing on tugboat.

I haven't yet looked at the code to see where the problem is. Will report in the PR after I do.

keiserjb commented 2 years ago

@argiepiano yes, I changed the config option from back_to_site to back_to_site_link and missed a spot in the module file, line 253

argiepiano commented 2 years ago

@keiserjb check your latest commit - you accidentally removed settings.php. Be sure to add it back and push another commit, so that we can test the latest changes.

argiepiano commented 2 years ago

OK, we have another problem now. The settings.php file you pushed actually contains some of your local configurations. Be sure it's the default settings.php file. You can get it by downloading the version 1.x of Backdrop

argiepiano commented 2 years ago

@keiserjb this is looking great. I have some comments (and @klonos did too).

One suggestion: the "Back to site" link currently points at the "real" path of the original page, rather than to the alias. It would be desirable if this functionality used the alias rather than the path. For example if your original page was example.com/my-page you want the link to point at that, rather than at, say, example.com/node/15, which is what's happening now.

I will suggest code to do this later today.

keiserjb commented 2 years ago

It's been busy in here but that's good. I will take a look at things tomorrow. I really think Backdrop needs this though.

keiserjb commented 2 years ago

Thoughts on setting this to enabled by default? I'd prefer it that way.

argiepiano commented 2 years ago

When this type of features are added, I think the consensus is that it should only be set as enabled in new installations. Existing installations should continue with the current look.

I believe that, to set it as enabled in new installations, all that's needed is to set the new flag to TRUE in the admin_bar.settings.json file - which is already there.

@backdrop/core-committers , can someone please "upgrade" @keiserjb so that the automated tests run whenever he pushes changes to his PR? Right now, every time he pushes something, someone with enough privileges has to approve the workflow.

@keiserjb you may want to "resolve" the comments you've already addressed, in the PR.

quicksketch commented 2 years ago

Thanks for the ping @argiepiano. I added @keiserjb to the Docs/Triage team.

bugfolder commented 2 years ago

Just took a look at this since it was on the design/UX agenda. I like this a lot! Kudos. Works great.

I agree with enable it only on new installations. But if we could flag it prominently as being available in the release notes, I bet a lot of current users would turn it on if they understand what it is.

keiserjb commented 2 years ago

I merely tweaked the work of @alexfinnarn and I realize now that's not fully reflected in my patch because I applied it manually so history was lost. Need to fix that.

argiepiano commented 2 years ago

I've reviewed the code and it looks good. Looking back at this thread, there are some suggestions from @BWPanda you should incorporate, specifically:

Only the first letter 'B' should be uppercase, the rest should be lowercase, same as other menu items (e.g. 'Back to site') [this is how D9 currently shows the Back to site link]
The setting for enabling/disabling this link says 'Add Back To Site Link' - the 'L' in 'Link' should be lowercase (e.g. 'Add Back To Site link')
The description for that option could use some re-wording; maybe something like "Changes the Home link/icon to a 'Back to site' link that links to the last non-admin page you visited."
...It's confusing to tick the box for the 'Back to site' link, but not have it appear because the 'Icon menu' is disabled (i.e. there's no indication the two are related or that one relies on the other). [Perhaps add some mention in the description of the Back to site setting explaining that "this link is only shown if the Icon menu is enabled below."

Responding to a concern from @klonos from 2018:

click on "Home" in the main menu (or any way to get you to the front page of the site)

click on any link on the admin menu -> "Back to site" link does NOT appear 👎🏼

This is actually by design. The Home icon is not replaced with "Back to site" if your original page before clicking on a admin page was the Home page. I think the original author of the PR had it this way because they thought it would not make sense to change "Home" by "Back to site" when in fact clicking it will take you "home" anyway.

jenlampton commented 2 years ago

I also just took a look at this, and I think it will be a very useful feature for editors and less experienced admins who are likely to get lost in the back-end. I'm a fan :)

My only concern is with the text Back to site.

This text indicates that we've somehow left the "site", when that's absolutely not the truth. We're still on the same site. The link just takes us back to a different page that we were on a while ago. That said, I don't have any recommendations for a replacement word for 'site' that would be less confusing. Back to front would be technically more correct, but likely even more confusing!

I don't think we should use Drupal as an "good example" for this text, but doesn't WordPress do something similar also? Do they use the same language? I wonder if Drupal copied WP without considering the distinction between "the site" and "the admin area" that exists in WP but not Drupal.

Only the first letter 'B' should be uppercase, the rest should be lowercase, same as other menu items (e.g. 'Back to site') [this is how D9 currently shows the Back to site link]

I was going to repeat this too...

The setting for enabling/disabling this link says 'Add Back To Site Link' - the 'L' in 'Link' should be lowercase (e.g. 'Add Back To Site link')

...but this text needs to match the capitalization of the link, so it should be Add Back to site link.

bugfolder commented 2 years ago

...doesn't WordPress do something similar also? Do they use the same language?

In WP, for logged-in admins, a black admin bar is always present at the top of the page. The top left "home" menu has its title be the name of your site (and links to the home page), and then, oddly, has an item title "Visit Site" that also takes you to the home page (not the non-admin page you were last on). See here:

So they do call the non-admin pages the "site".

I don't have a long history with WP, and didn't remember what they called it until I checked, but when I was testing this PR, it seemed pretty clear that "Back to site" suggested "Back to the public part of the site".

argiepiano commented 2 years ago

when I was testing this PR, it seemed pretty clear that "Back to site" suggested "Back to the public part of the site".

Me too. But maybe I'm used to it from D8/9

keiserjb commented 2 years ago

I was thinking of the WordPress way yesterday. But if we went for something different, perhaps using the word exit but then not all English speaking countries use it.

bugfolder commented 2 years ago

"Exit" strikes me as less suitable than "back to site". It might imply logging out, or entirely leaving the site. Plus, it seems more important to say where you're going to, rather than what you're leaving from.

Experienced developers would probably understand all of the above wordings (or would quickly figure it out). For the situation where the developer creates and configures the site, then turns it over to the local website administrator who just adds/edits content, at least some of those local administrators have the mental model that "the site" is the thing that the public sees, and the administrative interface is vaguely "something else for controlling the site" So for them, "Back to the site" actually fits their mental model.

I'm not claiming everyone thinks of it in that way, but I suspect that a higher percentage of the less-experienced administrators would, and those are the folks that this wording would be most beneficial for.

keiserjb commented 2 years ago

I agree that exit is the wrong word. If we wanted to be different perhaps Return to site.

keiserjb commented 2 years ago

@argiepiano I'm making the display of the Back to site checkbox dependent upon the Icon menu being checked.

...It's confusing to tick the box for the 'Back to site' link, but not have it appear because the 'Icon menu' is disabled (i.e. there's no indication the two are related or that one relies on the other). [Perhaps add some mention in the description of the Back to site setting explaining that "this link is only shown if the Icon menu is enabled below.

keiserjb commented 2 years ago

There appears to be a large problem in that the back to site link does not go away when the setting is unchecked and saved.

keiserjb commented 2 years ago

There appears to be a large problem in that the back to site link does not go away when the setting is unchecked and saved.

Fixed that.

Another question in here is what to do with the Back to site link when going back to the homepage. I would prefer this be cleaner and always say Back to site while on an admin page regardless of whether the link goes back home or not.

argiepiano commented 2 years ago

@keiserjb 🙏🏽 thank you for all these changes! I like the idea of hiding the Back to site link option when Icon menu is unchecked.

To make things even clearer, do you think moving the Back to site option lower, below the Icon menu checkbox option? See screenshot. I also changed the description a bit to make it clearer. I'm not expert on UIs so I'll defer to what others think

keiserjb commented 2 years ago

To get the checkbox in the components I need to change how the configuration works. Now it is separate. I started to do that but got stuck and then moved on.

From admin_bar.inc

$form['back_to_site_link'] = array(
    '#type' => 'checkbox',
    '#title' => t('Add Back to site link'),
    '#default_value' => $config->get('back_to_site_link'),
    '#description' => t('Changes the "Home" to a "Back to site" link that links to the last non-admin page you visited.'),
    '#states' => array(
          'visible' => array(
            ':input[name="components[admin_bar.icon]"]' => array('checked' =>
                                                                TRUE),
          ),
        ),
  );

  $form['components'] = array(
    '#type' => 'checkboxes',
    '#title' => t('Enabled components'),
    '#options' => array(
      'admin_bar.icon' => t('Icon menu'),
      'admin_bar.menu' => t('Management menu'),
      'admin_bar.search' => t('Search bar'),
      'admin_bar.page' => t('Page links'),
      'admin_bar.users' => t('User counts'),
      'admin_bar.account' => t('Account links'),
    ),
    '#default_value' => $config->get('components'),
    '#description' => t('These features will be enabled/visible in the administration bar. Untick the boxes to disable/hide them.'),
    'admin_bar.search' => array(
      '#states' => array(
        'visible' => array(
          ':input[name="components[admin_bar.menu]"]' => array('checked' => TRUE),
        ),
      ),
    ),
  );

  $form['components']['admin_bar.icon']['#description'] = t('A specialized menu that links to the home page and provides links to perform various other admin tasks (e.g.: clear caches, run cron, etc.).');
  $form['components']['admin_bar.menu']['#description'] = t('Displays the various administrative menu options in the admin bar.');
  $form['components']['admin_bar.search']['#description'] = t('A search box for quickly finding specific items in the Administration Menu.');
  $form['components']['admin_bar.page']['#description'] = t('Links to edit items that make up the current page (e.g. layout, theme).');
  $form['components']['admin_bar.users']['#description'] = t('A count of the people currently logged into the site.');
  $form['components']['admin_bar.account']['#description'] = t('Links to view and log out of the current user account.');

It would change how the config gets accessed in the module file and javascript.

keiserjb commented 2 years ago

@argiepiano I think I have the settings checkbox in the correct place now but not sure I implemented it the best way.

argiepiano commented 2 years ago

@keiserjb thanks for the new changes! 🙏🏽

I left some comments in the PR. We have some regressions of things you had fixed previously (e.g. the link should use the alias instead of the real path, and all packaging information is back in the PR. It seems to me that perhaps you started from scratch instead of continuing to work from your last commit.)

Also, someone commented above: the 'escape' graph seems lower resolution than the rest of the icons in the menu bar.

argiepiano commented 2 years ago

Hi @keiserjb and thank you for all the work! The patch is working great. I still have a couple more comments in the PR:

The issue of the low resolution for the back icon has not been resolved. I think this is an important one. We want the admin bar to look good.
I also had a suggestion about the behavior of the checkboxes. Typically, when box B depends on box A, and when B is hidden when A is false, we don't want to change the value of B when A is turned to false. The reason for this is that, if the user changes their mind about box A and turns it on again, we want the last value of B to be there when B is shown again. So, my suggestion is to not change the value of B (back to site link) in the submit handler, but rather check that A AND B are both true in the preprocess function before sending the setting to the Javascript code.

There are other minor style comments there too.

keiserjb commented 2 years ago

@argiepiano regarding the icons, I did double the resolution from 16px to 32px. But now I went back to the originals, turned the svg into png, saved them out to 64 x 64 and changed them from gray to white although there isn't much feathering. Photoshop skills have atrophied the last couple of years.

argiepiano commented 2 years ago

Hey @keiserjb! I had to clear the browser cache to see the new icon. The icon now looks good. 🎉

Sorry to be a pest, but there are a few coding standard issues now:

The js file has been completely reflowed with indentations of 4 spaces instead of 2
elseif should start in a new line (check the coding standards here)
To be consistent, the icon file should end with --white--64. Check out the names of the other icon files in core/modules/admin_bar/images (sorry for not noticing this one before!)

THANKS again for all the work and putting up with my suggestions. You know well how musicians can be in terms of details, being one of them yourself!

keiserjb commented 2 years ago

@argiepiano memories of Counterpoint class

argiepiano commented 2 years ago

@keiserjb thanks one more time. You addressed most of the comments. The js file now looks great.

Three things (hopefully the last ones). Two of them are really important:

The packaging information is still present in the PR, in file admin_bar.tests.info. We can't add that to PR. I suspect you started working from a release, rather than from the dev version 1.x. Keep in mind that PRs are applied to the 1.x version, and therefore it's important to start from that rather than from the latest release. In this case, the only problematic aspect is that you are including the packaging info (I think I found this 3 times so far, in three different reviews of this PR lol 🤣
You are creating a JS setting, $settings['frontpage'] in admin_bar_preprocess_page(). But that setting is never used by the JS code you included, nor anywhere else in the JS code of Backdrop. Can you please remove, or perhaps explain what you were intending to do?
Less important... no empty line between <?php and the first line of the doc block.

backdrop / backdrop-issues