Problem/Motivation

Steps to reproduce
Enable Settings tray module
Visit a non-admin page
Open the "Shortcuts" tray from the toolbar.
Click "Edit" in the toolbar to enter Editing mode.
The Shortcuts tray disappears but there is black area left.

Proposed resolution

Close all toolbar trays when entering edit mode.

Remaining tasks

User interface changes

API changes

Data model changes

CommentFileSizeAuthor
#25 2821263-25.patch616 bytestedbow
#23 2821263-23.patch606 bytesdroplet
#22 2821263-22.patch523 bytestedbow
#14 2821263-14.patch570 bytestedbow
#14 2821263-14-TESTS_ONLY.patch0 bytestedbow
#6 2821263-6.patch4.86 KBtedbow
#6 2821263-6-TESTS_ONLY.patch3.8 KBtedbow
#3 interdiff-2821263-2-3.txt4.42 KBtedbow
#3 2821263-3.patch4.86 KBtedbow
#2 2821263-close_toolbar.patch1022 bytestedbow

Comments

tedbow created an issue. See original summary.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Active » Needs review
StatusFileSize
new 2821263-close_toolbar.patch1022 bytes

Ok here is patch that closes any open toolbar item

If anybody is wondering why I am breaking up the #id based selector here:
$('#toolbar-bar').find('.toolbar-tab').has('.toolbar-tray.is-active').find('.toolbar-item').click();
It seems it is much faster: https://learn.jquery.com/performance/optimize-selectors/#id-based-selectors

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
StatusFileSize
new 2821263-3.patch4.86 KB
new interdiff-2821263-2-3.txt4.42 KB

Ok this patch does

  1. Changes closeToolbarTrays so it skips the contextual toolbar item. that is the 1 that is used for edit mode.
  2. Adds to \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks a check that if a tray is open when going into Edit mode it will be closed automatically. It checks user and admin menu toolbar items.
GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs review » Reviewed & tested by the community

Tested this and it works. Great job @tedbow

cilefen’s picture

cilefen commented
Title: If a toolbar items besides the admin menu is open when going into edit mode it is not closed. » If a toolbar item besides the admin menu is open when going into edit mode it is not closed.
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2821263-6-TESTS_ONLY.patch3.8 KB
new 2821263-6.patch4.86 KB

Reuploaing patch with a TESTS ONLY version.

The last submitted patch, 6: 2821263-6-TESTS_ONLY.patch, failed testing.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs review » Reviewed & tested by the community

Setting back to RTBC. #6 was just to upload the test only patch.
The other patch is the exact same as #3.

  • xjm committed d2effbc on 8.3.x 
    Issue #2821263 by tedbow: If a toolbar item besides the admin menu is...

  • xjm committed c9dbe65 on 8.2.x 
    Issue #2821263 by tedbow: If a toolbar item besides the admin menu is...
xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Thanks @tedbow. I also tested the fix manually and confirmed that both the shortcuts and the admin menu have the same behavior now.

Committed d2effbc and pushed to 8.3.x and 8.2.x. Thanks!

nod_’s picture

nod_
Primary language French
Location Lille
commented
Issue tags: +JavaScript

If it changes a JS file it needs to have the JavaScript tag.

nod_’s picture

nod_
Primary language French
Location Lille
commented
Status: Fixed » Needs work

We never use click on a jQuery object, always use .trigger('click'). The only time we use click() is on DOM objects.

Also if we can use CSS selectors, we should 

      .find('.toolbar-tab:not(.contextual-toolbar-tab)')
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Needs review
StatusFileSize
new 2821263-14-TESTS_ONLY.patch0 bytes
new 2821263-14.patch570 bytes

Ok here is fix.

The last submitted patch, 14: 2821263-14-TESTS_ONLY.patch, failed testing.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

I made mistake on #14 it did not actually have any test changes because the were already committed in #9 and #10.

Just need a review on the same JS change for #14

GrandmaGlassesRopeMan’s picture

GrandmaGlassesRopeMan
he/him
Primary language English
Location 🌎
commented
Status: Needs review » Reviewed & tested by the community

@tedbow

This looks good and addresses the feedback from #13.

+++ b/core/modules/outside_in/js/outside_in.js
@@ -89,10 +89,9 @@
+      .find('.toolbar-item').trigger('click');

This .trigger() could probably be on its own line to match the rest of the chaining style, but only a very minor nit.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2821263-14.patch, failed testing.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Reviewed & tested by the community

Random test failure,ArgumentPlaceholderUpdatePathTest, back to RTBC, rinse and repeat ;)

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: 2821263-14.patch, failed testing.

tkoleary’s picture

tkoleary commented
Status: Needs work » Reviewed & tested by the community
tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Reviewed & tested by the community » Needs review
StatusFileSize
new 2821263-22.patch523 bytes

Just a re-roll

+++ b/core/modules/outside_in/js/outside_in.js
@@ -89,10 +89,9 @@
-      .find('.toolbar-item').click();
+      .find('.toolbar-item').trigger('click');

This line was updated in another commit so not needed.

droplet’s picture

droplet commented
StatusFileSize
new 2821263-23.patch606 bytes

I can't reproduce the problem it described but to close the tray should be this way I think.

Status: Needs review » Needs work

The last submitted patch, 23: 2821263-23.patch, failed testing.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Status: Needs work » Fixed
StatusFileSize
new 2821263-25.patch616 bytes

@droplet yes the issue was committed in #9.

It was re-opened to address @nod_'s comment in #13

Thanks for your improvement!

I am going to make a follow up for your fix which looks better. Because it confusing to keep this issue open when it is fixed.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented

Whoops didn't mean to upload the patch in the last comment. Here is the new issue #2843901: Settings tray should use toolbar models to close the toolbar items

  • xjm committed d2effbc on 8.4.x 
    Issue #2821263 by tedbow: If a toolbar item besides the admin menu is...

  • xjm committed d2effbc on 8.4.x 
    Issue #2821263 by tedbow: If a toolbar item besides the admin menu is...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

tedbow’s picture

tedbow
he/him
Primary language English
Location Ithaca, NY, USA
commented
Component: outside_in.module » settings_tray.module

Changing to new settings_tray.module component. @drpal thanks for script help! :)