In the new Outside In experimental module the user should get a prompt that tells them they are now in edit mode.

The original design issue for this module such as #2762505: Introduce "outside in" quick edit pattern to core and #2732443: Finalize the behavior for triggering view/edit/build modes from the toolbar (and fix the "disappearing toolbar") call for something that looks more like a dialog with a standard close button.
Here is one of the examples from those issues:

The wording of this message will be discussed in separate issue: #2893350: Determine exact wording of "You are in edit mode." message when edit mode is enabled.

CommentFileSizeAuthor
#67 2773601-67.patch7.86 KBtedbow
#67 interdiff-65-67.txt7.25 KBtedbow
#65 2773601-65.patch12.02 KBtedbow
#65 interdiff-63-65.txt2.09 KBtedbow
#63 2893350-63.patch11.97 KBtedbow
#62 interdiff-61-62.txt2.19 KBtedbow
#62 2893350-62.patch12.28 KBtedbow
#61 2773601-61.patch12.25 KBtedbow
#61 interdiff-60-61.txt2.48 KBtedbow
#60 2773601-60.patch12.27 KBJo Fitzgerald
#60 interdiff-58-60.txt521 bytesJo Fitzgerald
#58 2773601-58.patch12.27 KBJo Fitzgerald
#52 2773601-52.patch13.03 KBtedbow
#52 interdiff-50-52.txt970 bytestedbow
#50 messages_style.png48.76 KBtedbow
#50 dialog_style_narrow_page.png61.93 KBtedbow
#50 dialog_style_wide_page.png52.5 KBtedbow
#50 2773601-50.patch13.04 KBtedbow
#50 interdiff-44-50.txt1.94 KBtedbow
#48 edit-mode-message-styling.png60.42 KByoroy
#48 edit-mode-message.png35.4 KByoroy
#45 extra-toolbar-rows.png1.24 MByoroy
#44 773601-44.patch13.14 KBtedbow
#44 interdiff-32-44.txt2.77 KBtedbow
#44 dialog_msg_no_title.png36.11 KBtedbow
#41 message-edit-mode-4.png385.16 KByoroy
#41 message-edit-mode-1.png355.61 KByoroy
#41 message-edit-mode-3.png302.93 KByoroy
#41 message-edit-mode-2.png463.54 KByoroy
#39 2773601-39.patch13.22 KBtedbow
#39 interdiff-2773601-33-39.txt723 bytestedbow
#38 2773601-38.patch13.19 KBtedbow
#38 interdiff-2773601-33-38.txt694 bytestedbow
#33 edit_message_dialog.png56.79 KBtedbow
#33 2773601-33.patch12.95 KBtedbow
#33 interdiff-2773601-32-33.txt4.71 KBtedbow
#32 2773601-32.patch12.89 KBtedbow
#32 interdiff-2773601-29-32.txt3.53 KBtedbow
#29 2773601-29.patch11.45 KBtedbow
#29 interdiff-26-29.txt4.97 KBtedbow
#26 2773601-26.patch11.63 KBtedbow
#26 interdiff-2773601-23-26.txt3.68 KBtedbow
#23 2773601-23.patch11.86 KBtedbow
#23 interdiff-21-23.txt1.47 KBtedbow
#21 2773601-21.patch11.8 KBtedbow
#21 interdiff-19-21.txt9.47 KBtedbow
#19 2773601-19.patch6.14 KBtedbow
#16 interdiff-14-16.txt1.48 KBtedbow
#16 2773601-16.patch4.55 KBtedbow
#14 interdiff-11-14.txt1.07 KBtedbow
#14 2773601-14.patch4.19 KBtedbow
#11 2773601-11.patch4.1 KBdrpal
#8 2773601-8.patch4.2 KBtedbow
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

tedbow created an issue. See original summary.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.0-beta1 was released on August 3, 2016, which means new developments and disruptive changes should now be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

xjm’s picture

Title: Display "You are now in edit mode" prompt when user enters edit mode. » Display "You are now in edit mode" prompt when user enters edit mode
Version: 8.3.x-dev » 8.2.x-dev
Component: contextual.module » quickedit.module
tkoleary’s picture

Is it possible to make this part of a tour that is toggled on by default?

naveenvalecha’s picture

Component: quickedit.module » outside_in.module
SKAUGHT’s picture

tkoleary’s picture

As an MVP for this patch could we simply create a status message for this?

tedbow’s picture

Status: Active » Needs review
FileSize
4.2 KB

Ok here is patch that adds the message. I think will probably want to change the actual message to describe what it is.

I put a todo about refactoring once #77245: Provide a common API for displaying JavaScript messages is committed.

Right now if you click the message it disappears. This is inline JS which I imagine should be changed.

I wasn't sure how to insert it with unknow page markup so I used before main-canvas.

This also includes a test to make sure the message appears and is removed when leaving edit mode.

tedbow’s picture

+++ b/core/modules/outside_in/js/outside_in.js
@@ -148,8 +166,9 @@
-      if (editMode) {
-        setEditModeState(true);
+      // Enter Edit mode if set in local storage and not currently in edit mode.
+      if (editMode && !isInEditMode()) {
+        setEditModeState(true, false);

I added this extra check because displaying the message pointed out the on page load if already in Edit mode setEditMode would be called over and over again. Presumably this is every time a contextual link is added.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

drpal’s picture

Version: 8.3.x-dev » 8.4.x-dev
FileSize
4.1 KB

Rerolled the patch from #8.

drpal’s picture

+++ b/core/modules/outside_in/js/outside_in.js
@@ -118,20 +118,36 @@
+    var htmlMsg = '<div class="settings-tray-messages messages messages--status" onclick="this.parentNode.removeChild(this);" role="alert">' + ' ' + editMsg + '</div>';

I'm not thrilled about having simple string represent complex functionality. Perhaps a better way to construct this and attach the event listener.

drpal’s picture

Status: Needs review » Needs work
tedbow’s picture

Status: Needs work » Needs review
FileSize
4.19 KB
1.07 KB

I'm not thrilled about having simple string represent complex functionality. Perhaps a better way to construct this and attach the event listener.

@drpal I want you to thrilled about this patch ;)
Moved to an event listener and not adding the element as a big string now.

drpal’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -118,20 +118,41 @@
    +    var msgDiv = document.createElement('div');
    

    +1 for vanilla js.

  2. +++ b/core/modules/outside_in/js/outside_in.js
    @@ -118,20 +118,41 @@
    +    $(msgDiv).insertBefore('div.dialog-offcanvas__main-canvas').hide().fadeIn('slow');
    

    Do we need to call drupal.debounce() here since we're adding an element to the page? Additionally drupal.annoucne() for a11y?

@tedbow PS. I am thrilled. U+1F609

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.55 KB
1.48 KB

Ok add anounce() and debounce()

I am not sure how we handle adding the correct css.
Also if you are scrolled down on the page you won't see the message.

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

tedbow’s picture

Status: Needs review » Postponed

I think we should postpone this on #77245: Provide a common API for displaying JavaScript messages
It doesn't make sense to do this twice.

tedbow’s picture

Status: Postponed » Needs review
FileSize
6.14 KB

Ok #77245: Provide a common API for displaying JavaScript messages may or may not be committed by the time 8.4.0 comes out.

We can add this here and then refactor if needed.

This mostly a re-roll of #16 plus

  1. Necessary es6 changes
  2. Removed message when exiting edit mode(can still be clicked on to remove at any time.
  3. Updates test coverage.
drpal’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript
  1. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -3,7 +3,7 @@
    +(function ($, Drupal, debounce) {
    

    Depending on what other Drupal properties we're using, this might be an opportunity for destructing.

  2. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -118,12 +118,30 @@
    +  function displayEditMessage() {
    

    This could use some additional documentation.

  3. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -118,12 +118,30 @@
    +  function setEditModeState(editMode , displayMessage = false) {
    

    Extra space before the ,. Did eslint not pick this up?

  4. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -179,6 +202,8 @@
    +      $('#settings-tray-edit-message').remove();
    

    Cache this result of this selector possibly?

tedbow’s picture

Status: Needs work » Needs review
FileSize
9.47 KB
11.8 KB

1. Adding destructing on Drupal announce, ajax, debounce, toolbar, behaviors, attachBehaviors(), t() are used.
2. add more comments
3. Yes eslint picked this up. fixed
4. This selector is only used once. It is called multiple times if the user goes in and out of edit mode. but the message element will be different because is created each time.

Also I had to explicitly add toolbar/toolbar as a dependency to the outside_in library not sure if this has to do with destructing.
UPDATE: figured out why destructing JS change probably cause an error unless a dependency was added for toolbar/toolbar for the oustide_in library.

Since without destructing we didn't pass in Drupal.toolbar explicitly we just passed in Drupal and referenced Drupal.toolbar later, at the time the file loaded the toolbar property didn't have to be on the Drupal object. It only had to be there later when edit mode was invoked. So if toolbar.js was loaded later that would still work.

Using destructing Drupal.toolbar needs to be defined before the file is loaded so toolbar.js must be loaded first.

really toolbar/toolbar should have always been a dependency of oustide_in library but we just never noticed before.
So the really we should dependencies on drupal.announce and drupal.debounce also.

drpal’s picture

Status: Needs review » Needs work
  1. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -2,8 +2,7 @@
    +(function ($, {t, announce, debounce, ajax, attachBehaviors, toolbar, behaviors}) {
    

    Spaces around, {}.

  2. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -41,8 +40,8 @@
    +      announce(
    

    No new line.

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.47 KB
11.86 KB

1. Fixed
2. Fixed

Also had needed dependencies to drupal.outside_in. They should have already been there but didn't notice until similar problem found in #21

drpal’s picture

Status: Needs review » Reviewed & tested by the community
+++ b/core/modules/outside_in/js/outside_in.es6.js
@@ -2,8 +2,7 @@
+(function ($, { t, announce, debounce, ajax, attachBehaviors, toolbar, behaviors }) {

This introduces a new pattern of destructing the Drupal object. 👍

droplet’s picture

Status: Reviewed & tested by the community » Needs work

We couldn't alias Drupal.t because it can't parsed by PHP.

Indirect issue but it tells how Drupal.t works in JS:
#2889600: [regression] Restore \LocaleJavascriptTranslationTest test coverage and keep testing processed JS file

In short word:
PHP matches Drupal.t
@see \_locale_parse_js_file

tedbow’s picture

Status: Needs work » Needs review
FileSize
3.68 KB
11.63 KB

@droplet thanks for the information. I definitely would never have thought of that.

So this patch doesn't alias Drupal.t.

But this results in
})(jQuery, Drupal, Drupal);
Which looks strange, but it does allow use to do destructiing.

As a follow up I think any Javascript function that doesn't allow aliasing because of something like this should have that clearly documented in the JSDoc. I looked at the JSDoc for Drupal.t and there is no way you would ever figure this out without looking at PHP code.

droplet’s picture

Status: Needs review » Needs work

Which looks strange, but it does allow use to do destructiing.

Passing the variable isn't a must. We can drop it.

(function ($, { announce, debounce, ajax, attachBehaviors, toolbar, behaviors }) {
//....
})(jQuery, Drupal);
  1. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -115,12 +112,36 @@
    +  function displayEditMessage() {
    +    const editMsg = Drupal.t('You are now in edit mode.');
    +    const msgDiv = document.createElement('div');
    +    msgDiv.setAttribute('id', 'settings-tray-edit-message')
    +    msgDiv.setAttribute('class', 'settings-tray-messages messages messages--status');
    +    msgDiv.setAttribute('role', 'alert');
    +    msgDiv.innerHTML = editMsg;
    +    $(msgDiv).insertBefore('div.dialog-off-canvas__main-canvas').hide().fadeIn('slow');
    +    announce(editMsg);
    +    $(msgDiv).on('click.outsidein', e => $(e.target).remove());
    

    I think we should make it Drupal.theme

    and use prefix with `.js-` so that will not removed by themer..etc

  2. +++ b/core/modules/outside_in/js/outside_in.es6.js
    @@ -132,6 +153,11 @@
    +        const displayFunction = debounce(displayEditMessage, 300, true);
    +        displayFunction();
    

    This doesn't debounced I think. If you calling setEditModeState, it's either skipped or redefined. (with or without it, no diff)

    You could see the problem by:

            displayFunction();
            displayEditMessage();
            displayEditMessage();
            displayEditMessage();
            displayEditMessage();
            displayEditMessage();
    

    Actually, you only need to add a condition, eg:

    if(!$('#settings-tray-edit-message').length) {
     displayEditMessage()
    }
    

    BTW, I can't imagine a use case you will call it twice in a short time

droplet’s picture

Also, I think to click the message to hide it isn't quite good. They will think it's exit editing.

And there's a big toolbar with large White Spacing on the right. Can't put it over there?

If it's a static message, I will add few more words. e.g.:

You are now in edit mode. <button> Exit it </button>

tedbow’s picture

Status: Needs work » Needs review
FileSize
4.97 KB
11.45 KB

Ok removing passing in Drupal twice.
#27.2
I added this check

  function displayEditMessage() {
    if ($('#settings-tray-edit-message').length) {
      return;
    }

So the displayEditMessage() won't try to display the method if it is already shown.
Removed dependence on debounce.

RE #27.1 and #28
I look back at the original design issue for this module such as #2762505: Introduce "outside in" quick edit pattern to core and #2732443: Finalize the behavior for triggering view/edit/build modes from the toolbar (and fix the "disappearing toolbar")

The designs call for something that looks more like a dialog with a standard close button.
Here is one of the examples from those issues:

So have changed this to use Drupal.dialog.
So

I think we should make it Drupal.theme

and use prefix with `.js-` so that will not removed by themer..etc

Because we using Drupal.dialog this not longer applies

I think this takes of #28 because we much closer to the original designs that were user tested this way.

droplet’s picture

Ahh. You using a secret version. haha. The GIF behavior diff to patch. If I using it frequently, it's a bit annoying.


+++ b/core/modules/outside_in/outside_in.libraries.yml
@@ -20,6 +20,9 @@ drupal.outside_in:
+    - core/drupal.debounce

Needs to remove it

Status: Needs review » Needs work

The last submitted patch, 29: 2773601-29.patch, failed testing. View results

tedbow’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
3.53 KB
12.89 KB

Ahh. You using a secret version. haha.

Sorry updated the issue summary.

1. Removed dependency on core/drupal.debounce

2. The tests in #29 failed because now that we are using a dialog it may be in front of other elements that the test is trying to interact with.

I added \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::closeMessageDialog(). This closes the dialog whenever edit mode is enabled explicitly or when a block for is opened which also may enable edit mode if it is not already enabled.

I added waitForElementVisible() for this to be sure the element was actually displayed before clicking
I also added a waitForNoElement because it could cause random failures on drupalCI if this still in process of being removed before the test proceeds.

3. add CSS to make the dialog appear more like the designs by remove the border and background color for the title bar.

4. We to figure if we need the second part of the message or if we can do that in a follow up.

tedbow’s picture

Ok I added longer message from the original designs. I would rather not debate the text of the this message right now because that is more of UX issue and this issue still needs to figure out the technical aspect of displaying the message first.
I do think we will need a longer message that describes what edit mode is. We can figure out the exact text after we figure out the technical aspects.

But having the longer message allows use to get rid of the special styles for this dialog because we have a title and a message.

Here is screenshot:

drpal’s picture

+++ b/core/modules/outside_in/js/outside_in.es6.js
@@ -2,8 +2,7 @@
+(function ($, { announce, ajax, attachBehaviors, toolbar, behaviors }) {

Can we add some documentation about why the rest of these values can be assigned with destructing but Drupal.t cannot?

tedbow’s picture

@drpal I feel we should add a comment instead to every function we cannot alias: #2893358: Add documentation Drupal.t() and other JS functions that cannot be aliased because they are scanned for in PHP

droplet’s picture

I'm thinking of another way to handle it. Doc it still increasing of review workloads.

#2893361: Aliasing of `Drupal.t` and `Drupal.formatPlural` in JS

tedbow’s picture

Ok adding comment asked for #35 which includes a @todo point to #2893358: Add documentation Drupal.t() and other JS functions that cannot be aliased because they are scanned for in PHP

I don't think we need to wait on that issue though.

tedbow’s picture

Made a type in that comment. Also a @see to the function that scans for Drupal.t

The last submitted patch, 38: 2773601-38.patch, failed testing. View results

yoroy’s picture

Is this a custom solution for only edit mode? I ask because I have designs that make it more specific to this situation:

Current proposal with a title and a message seems a bit much, especially when there's empty space next to the "Editing" button:

Could we use that empty space in the toolbar?


Alternatively, what about stripping the current application down to its bare essentials:

tedbow’s picture

@yoroy the current patch uses the standard Drupal dialog system(as does the tray itself) so it is not a custom solution in that sense. It is just positioned differently

Could we use that empty space in the toolbar?

The problem I see with that is that we are not sure what might be placed in the toolbar in edit mode in the future. Currently it the Place Button remains in the toolbar. There maybe other other things in the future that would go in the toolbar that only show up in edit mode.

Alternatively, what about stripping the current application down to its bare essentials:

I like this last approach.

The patch in #26 and above was taking similar approach before we started to use the dialog system.

yoroy’s picture

Ok, so lets not over-optimize right now and see how we can tweak the design of the default dialog implementation (and not put the message inside the toolbar).

tedbow’s picture

Ok here is a patch that used the dialog system but does not have a special title section

dialog without title section

I started from patch #32.

Then added:

  1. CSS to remove special title section
  2. Comment about destructing from #39
  3. Fixed the test so it just clicks on the dialog itself to close it(this is supported by our dialog system)
yoroy’s picture

FileSize
1.24 MB

During today's UX meeting Gabor very rightfully pointed out that we already have two quite different ways of showing messages/functionality in a similar space:

Lets look into standardizing the look and feel of these instead of introducing yet another version. Either way, the single line of text in a dismissable dialog that's tucked closely under the toolbar is a good step in the right direction.

(We also discussed that a automatically disappearing message (after x seconds) could be useful here, which would be an interesting addition to the standard dialog capailities but that's out of scope for this specific issue.)

tedbow’s picture

Ok @drpal reminded me that once #77245: Provide a common API for displaying JavaScript messages lands we will just be to do something like
Drupal.message('#top-message-aread').add('You are in edit mode');

Lets look into standardizing the look and feel of these instead of introducing yet another version.

So once we have a common way to output display message via JS then this look like any other message on the site.

We the could a dismissable behaviour to all our messages whether they come the server or JS. So we could add something like
Drupal.message('#top-message-aread').add('You are in edit mode', {dismissable: true);

The point is that the solution here likely will improved and standardized using #77245.

+++ b/core/modules/outside_in/js/outside_in.es6.js
@@ -115,12 +116,47 @@
+   * @todo Refactor to use Javascript Message API https://www.drupal.org/node/77245

We already have an existing @todo point to this issue in the patch!

Either way, the single line of text in a dismissable dialog that's tucked closely under the toolbar is a good step in the right direction.

So I think our current patch is good then?

drpal’s picture

Status: Needs review » Reviewed & tested by the community

🚀🚀

yoroy’s picture

Issue summary: View changes
Status: Reviewed & tested by the community » Needs work
FileSize
35.4 KB
60.42 KB

Sorry, I still want to be pragmatic here, but we're also trying to actually stabilize things. I think that also means we should be intentional about how things look. Especially with 2 related designs in place (#45) we should not introduce yet another styling, even if only temporary. We're stabilizing *now* and I'm reluctant to make finessing this dependent on an 11 year issue making it in before 8.4.

So I had a stab at styling the dialog message similar to the "exit block region demonstration" tab underneath the toolbar:

I took a screenshot of the CSS I hacked into it via Firebug :)

This is likely a messy fix code-wise but for the actual appearance I think it would be an improvement. Thoughts? Reasonable doable?

droplet’s picture

I actually have few questions:

- Will this message improve the UX?
- What if we changing "Editing" to "< Exit Edit Mode" (we already have this style in admin section)
- What does user think about of the Close icon? Close this message Or Exit Edit Mode.

tedbow’s picture

@droplet
- Yes, I think the message will improve UX but probably a different message like "Click a page element to edit it." that is why I created #2893350: Determine exact wording of "You are in edit mode." message when edit mode is enabled.. I think if we try to do both the JS implementation and wording in the same issue will never come to an agreement 😜
- I think "Exit Edit Mode" might be a good idea but I think that would in addition to wording that explain what the user should to in edit mode via ^^. So I think this should be a separate issue if we want to change the "Editing" text in the toolbar
- I think it is common pattern on the web to inform the user of something with a dialog and allow them to dismiss the message. I don't think the user expect dismissing the message would reverse what the message informed them of.

@yoroy

we should not introduce yet another styling, even if only temporary.

While agree we don't want to introduce a new styling without reason I don't know that copying the Exit link on the Block demonstration page is the best solution. Is this page actually used much?
The UX study #2497361: [meta] Fix issues found during UMN Usability Testing 2015 found

The "demonstrate block regions" link was completely missed. If that had been used, the user might have better understood block regions.

(#2514150: Advertise the block region demonstration in a more prominent way was opened to solve this but I was surprised to find was closed because Block Place "solves" the demo regions problem)

The fact is the styling provided by the block module for this probably only looks good in bartik. No core theme actually override the styles for the class .block-demo-backlink
Doing a quick google search tells me not many contrib theme actually took the time to style this.
Even if contrib or custom themes did style this because the block module uses a very specific class, .block-demo-backlink, our use would not get the same styling.
So we would either have to:

  1. Copy the styling from the block module. If we did this any theme that already overrode .block-demo-backlink to make it look in their theme would immediately have a different styling between "You are in edit mode" and "Exit block demo"(because we can't use .block-demo-backlink). Any theme that didn't override .block-demo-backlink would get the same look but my guess this is because the Block demo page is not a page their users see much so they didn't bother to make it look good in the first place. So they still have to override our styles.
  2. Create neutral styling over our own that we think will look good in many themes. Then have defeated the purpose of not introducing a new styling in core.

The fact is right now because #77245: Provide a common API for displaying JavaScript messages we have no common styling for messsage that appear after the user is already on the page because there is common way to do this. The only place I can think of is when you have unsaved changes in form you get a message style warning.

Here are a couple ideas to not introduced a new styling.

Idea #1: Dialog approach

Make this message look more like a standard dialog. Then we are using that a styling that already exists. I have attached patch that does this.

Here is what it looks like:

This is just using our standard dialog but with only the title bar showing, only the positioning has changed. One advantage of this method is that front-end themes will already styled modals/dialogs so we should just pick up the styles that they apply to dialogs and will get a more uniform look.

Here is the same dialog but with the page in narrower width.

You will notice that dialog actually because wider. But again this is because we are using the standard dialog and whatever styles are applied to dialogs will be picked up.

In this way we are not introducing a new styling.

Idea #2: Messages approach

Revert back to the idea in patch #26

This uses existing messages styles but does put them in new place.

If we went this way we would probably want to add a new "X" for closing the message.

Status: Needs review » Needs work

The last submitted patch, 50: 2773601-50.patch, failed testing. View results

tedbow’s picture

Status: Needs work » Needs review
FileSize
970 bytes
13.03 KB

Forgot to update the test coverage.

andrewmacpherson’s picture

Remind myself to look at accessibility of this in detail.

Skim-reading the patch, it looks like it's making good use of Drupal.announce().

Also, I really like that the message appears in physical proximity to the edit button. That's good for people using screen magnifier AT, much less risk of it appearing outside their viewport.

Wim Leers’s picture

I think both this issue and #2889747: Determine if elements should be removed from the toolbar during edit mode. can be solved by making it far more visually clear that this is a mode with limited capabilities. If there were a visual (color) connection between the "Editing" tab in the top left and the blocks you can actually click to edit, this problem would go away.

Regardless of what you think of my proposal, these issues are closely related.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

tedbow’s picture

Component: outside_in.module » settings_tray.module

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

tedbow’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
Jo Fitzgerald’s picture

Status: Needs work » Needs review
Issue tags: -Needs reroll
FileSize
12.27 KB

Re-rolled.

Status: Needs review » Needs work

The last submitted patch, 58: 2773601-58.patch, failed testing. View results

Jo Fitzgerald’s picture

Status: Needs work » Needs review
FileSize
521 bytes
12.27 KB

Add missed correction of class from "outside-edit-message" to "settings-tray-edit-message".

tedbow’s picture

@Jo Fitzgerald thanks for the re-roll!

re @Wim Leers' idea about

If there were a visual (color) connection between the "Editing" tab in the top left and the blocks you can actually click to edit, this problem would go away.

I do think this an interesting idea but also don't want to just abandon the what was test in UX mockup before module code was ever written. I am not sure but I think we should go back to UX folks/meeting if we want to change something like this. Interested in what @yoroy thinks about this.

So as the patch stands now we are using the dialog system as described in #50. We just need sign off that this is correct way to go to mark this RTBC. In #47 @drpal marked this as RTBC. So I think we are good Javascript wise. In #49 @droplet had some concerns but no problems noted with the code.

In #48 @yoroy raised

we should not introduce yet another styling, even if only temporary.

I now don't think we are introducing new styling as detailed in #50

there were a couple UX concerned raised by @droplet in #49 and @Wim Leers in #50.

Both these are diverging from the UX that was actually tested in mockup with users before this module was written. I don't think we should be doing this in individual issues in the SEttings Tray modules. I think we should implement the UX that was tested.

I think if we are going to change the UX we should do it only after implementing the original UX and seeing that there is something wrong with it from actual users.

Couple remaining things on the patch. Uploading a another patch with these.

+++ b/core/modules/settings_tray/js/settings_tray.js
@@ -54,7 +54,7 @@
-      dialogClass: 'outside-edit-message'
+      dialogClass: 'settings-tray-edit-message'

This change should be have been made to settings_tray.es6.js and then the file should have been built as described here: https://www.drupal.org/node/2815083

  1. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -60,12 +64,46 @@
    +    $(msgDiv).on('click.outsidein', e => $(e.target).remove());
    

    Need to update click event.

  2. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -123,7 +165,8 @@
    -      disableQuickEdit();
    +      disableQuickEdit();// Remove edit mode message.
    +      $('#settings-tray-edit-message').remove();
    

    The comment just got moved to after the last line.

tedbow’s picture

I updated the edit message to "Click a page element to edit it." because that is what was determined in #2893350: Determine exact wording of "You are in edit mode." message when edit mode is enabled.

tedbow’s picture

Just re-rolled this patch

drpal’s picture

Status: Needs review » Needs work
Issue tags: +JavaScript
  1. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -2,10 +2,14 @@
    +(function ($, { announce, ajax, attachBehaviors, toolbar, behaviors }) {
    

    Arrow function here. Additionally attachBehaviors is declared, but never used.

  2. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -59,13 +63,47 @@
    +  /**
    +   * Display a message informing the user that they have entered edit mode.
    +   *
    +   * This message appear at the top of the page below the toolbar.
    +   * The user will be able to click on the message to remove it.
    +   * It will also be removed when exiting edit mode.
    +   *
    +   * @todo Refactor to use Javascript Message API https://www.drupal.org/node/77245
    +   */
    

    We probably want to document that this returns early if there are no items returned.

  3. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -59,13 +63,47 @@
    +    msgDiv.setAttribute('id', 'settings-tray-edit-message')
    

    Missing semicolon.

  4. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -59,13 +63,47 @@
    +    Drupal.dialog(msgDiv, {
    

    We're destructing everything else from Drupal, why not dialog?

tedbow’s picture

Status: Needs work » Needs review
FileSize
2.09 KB
12.02 KB

1. fixed
2. It returns early if the message div is already on the page. Added a comment before the return statement
3. fixed
4. using destructing now for Drupal.dialog

tim.plunkett’s picture

Status: Needs review » Needs work

The main portion of this patch looks great!

Here are a couple things I noticed:

  1. +++ b/core/modules/settings_tray/css/settings_tray.module.css
    @@ -21,3 +21,7 @@
    +  display: none !important;
    

    !important is a huge red flag. Is it really necessary here?

  2. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -2,10 +2,14 @@
    + * The Drupal.t() function is used in this file but cannot be destructed here
    + * aliased because Javascript files are scanned for calls to Drupal.t().
    + * @todo Destructure Drupal.t() if in possilbe in https://www.drupal.org/node/2893361
    ...
    -(function ($, Drupal) {
    +(($, { announce, ajax, dialog, toolbar, behaviors }) => {
    
    @@ -32,7 +36,7 @@
    -    $(Drupal.toolbar.models.toolbarModel.get('activeTab')).trigger('click');
    +    $(toolbar.models.toolbarModel.get('activeTab')).trigger('click');
    
    @@ -153,7 +198,7 @@
    -    Drupal.ajax.instances
    +    ajax.instances
    
    @@ -182,7 +227,7 @@
    -     * instances in Drupal.ajax.instances for them to work with Edit Mode.
    +     * instances in ajax.instances for them to work with Edit Mode.
    
    @@ -214,9 +259,7 @@
    -      Drupal.announce(
    -        Drupal.t('Exited edit mode.'),
    -      );
    +      announce(Drupal.t('Exited edit mode.'));
    
    @@ -230,7 +273,7 @@
    -  Drupal.behaviors.toggleEditMode = {
    +  behaviors.toggleEditMode = {
    
    @@ -253,4 +296,4 @@
    -}(jQuery, Drupal));
    +})(jQuery, Drupal);
    

    All of this is out-of-scope. They are great changes, but should be done in a dedicated issue. Confirmed with @drpal that it's okay to punt any ES6ification

  3. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -59,13 +63,48 @@
    +   * Display a message informing the user that they have entered edit mode.
    ...
    +   * @todo Refactor to use Javascript Message API https://www.drupal.org/node/77245
    

    Not sure if the standard differs from PHP, but if this weren't JS code I'd say the first line should be "Displays" and that the @todo should be wrapped to 80 characters.

  4. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -59,13 +63,48 @@
    +   *  True if the message about entering edit mode should be shown.
    

    One character short on the indent

tedbow’s picture

Status: Needs work » Needs review
FileSize
7.25 KB
7.86 KB

1. Because jQuery UI Dialogs add inline CSS it is hard to override. Basically we have dialog that only has a title. So we want not to display the content area. I have changed this to use Javascript to set the CSS.

2. Glad to remove these extra changes!
3. change to "Displays" also wrapped at 80. The standard is under discussion at #2924755: Set max line length for JavaScript code comments to 80 (rather than 100)
4. Fixed

tim.plunkett’s picture

This looks great!
I see that there are new Drupal.announce() calls, which is good. But I'm not qualified to remove the "needs accessibility review" tag, so I won't RTBC it just yet.
But after that review, I think this is good to go.

tedbow’s picture

Status: Needs review » Needs work

Found a problem. I am not a keyboard navigation expert but...
The dialog we use for this is not tabbable. So you cannot close it via the keyboard.

This is because the Contextual module restrains tabbing to the contextual links and the toolbar via:
tabbingContext = Drupal.tabbingManager.constrain($('.contextual-toolbar-tab, .contextual'));

So we could add 'contextual' class to our dialog but that is a hack. Also the contextual module announces a message

Tabbing is constrained to a set of 8 contextual links and the edit mode toggle.
Press the esc key to exit.

So even if were to add our dialog into the tabbing context this message would be wrong.

Wim Leers’s picture

Drive-by review:

  1. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -59,13 +59,51 @@
    +    if ($('#settings-tray-edit-message').length) {
    

    Channeling @nod_: why not use document.querySelector()?

  2. +++ b/core/modules/settings_tray/js/settings_tray.es6.js
    @@ -124,6 +166,8 @@
    +      // Remove edit mode message.
    

    Nit, for clarity: s/edit mode/"edit mode"/

tedbow’s picture

Status: Needs work » Needs review

#69 identifies problem with displaying the dialog message. To fix this we would need non-trivial to contextual module and probably to Drupal.tabbingManager

The problem with doing this is likely that if #77245: Provide a common API for displaying JavaScript messages was finished we would not be using a dialog at all for this message. So we would making changes to contextual for a temporary problem.

I would propose that we postpone this issue on #77245 for these reasons:

  1. Not to make changes to contextual and Drupal.tabbingManager
  2. We are currently giving the user visual clues they can "Click a page element to edit it.". Namely the dotted lines around the clickable elements and the hover changes for the elements that are clickable
tedbow’s picture

Status: Needs review » Postponed

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.