Skip to content

Conversation

Addison-Stavlo
Copy link
Contributor

@Addison-Stavlo Addison-Stavlo commented Nov 7, 2019

Edit

This PR has been edited to reduce much of the functionality shown/requested below, but I am leaving the original PR description for transparency. A comment below describes the limited changes now being requested. TLDR - Adds the current "label" attribute of the close button as a visible child.

-----------original PR description below----------

Description

I have added two new preferences to the core/edit-post store to be used by the FullscreenModeClose component. Additionally this component has been updated to allow some conditional rendering for these preferences. These new preferences are:

1 - alwaysShowCloseButton - (default: false) when turned on the close button will be rendered in non-fullscreen mode and at all screen widths. In working with dotcom Full Site Editing, we found it would be useful to allow access to this close button from the windowed wp-admin mode.

2 - showCloseButtonLabel - (default: false) when turned on the label will be rendered alongside the chevron icon. In new user testing sessions it was found that the unlabeled chevron did not meaningfully convey where that button would lead, causing users to fumble for a while in fullscreen mode not being sure of how to go back to the page selection screen.

How has this been tested?

  1. Run this PR.
  2. Navigate to open the page or posts editor.
  3. Verify initial behavior is unchanged, that close button:
    a. is visible in the fullscreen editor.
    b. is not visible in windowed editor.
    c. does not render a label.
    d. is not rendered below medium screen width ( 782px ).
  4. In the browser console, run wp.data.dispatch( 'core/edit-post' ).toggleFeature( 'alwaysShowCloseButton' )
  5. Verify that the close button now renders when:
    a. editor is in windowed mode.
    b. screen width is below medium width ( 782px ).
  6. In the browser console, run wp.data.dispatch( 'core/edit-post' ).toggleFeature( 'showCloseButtonLabel' )
  7. Verify that the close button now displays its label to the right of the chevron icon.

Screenshots

Initial Windowed View
Screen Shot 2019-11-07 at 6 08 12 PM

alwaysShow toggled on in windowed mode
Screen Shot 2019-11-07 at 6 08 31 PM

alwaysShow toggled on in fullscren mobile width
Screen Shot 2019-11-07 at 6 09 19 PM

showLabel toggled on in windowed mode
Screen Shot 2019-11-07 at 6 08 45 PM

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.

Copy link
Member

@noahtallen noahtallen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice changes! I think this will work really well. It'd be great if some folks with more Gutenberg context could take a look and see if this is the best place to add this extension point :)

function FullscreenModeClose( { isActive, postType } ) {
if ( ! isActive || ! postType ) {
function FullscreenModeClose( { isActive, postType, isAlwaysShown, isLabeled } ) {
if ( ! ( isActive || isAlwaysShown ) || ! postType ) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could also use the ifCondition HOC to hide the post based on these conditions. See something similar here: https://github.com/WordPress/gutenberg/pull/17117/files#diff-6bc1caaad4f05a3668923c347ab3834eR57

You basically do ifCondition( props => something); and, something should be true if you want the component to display, and false if you want the component to be hidden.

@noahtallen noahtallen added [Package] Edit Post /packages/edit-post [Feature] Extensibility The ability to extend blocks or the editing experience labels Nov 11, 2019
@noahtallen
Copy link
Member

cc @epiqueras @youknowriad who would be a good person to give some feedback on these changes?

Copy link
Contributor

@epiqueras epiqueras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except for the naming comment, it looks good to me.

@youknowriad, is there a concern around adding potentially unnecessary "features"?

@youknowriad
Copy link
Contributor

@epiqueras I agree that this seems way too specific and complex for a use case that is not defined clearly. can we learn more about the use-case here?

@noahtallen
Copy link
Member

Absolutely, @youknowriad. I'll speak from the context of WordPress.com and feedback we've gotten from designers/users/happiness engineers.

Here are some screenshots.

  1. There is no back button on mobile viewports. People don't seem to be comfortable always relying on the browser back button here.

Screen Shot 2019-11-12 at 10 27 41 AM

  1. There is no back button on non-full-screen viewports. Same issue as above, but also adding that if you are navigating between two pages of the block editor, it may not be clear what you have to do to get back.

Screen Shot 2019-11-12 at 10 27 07 AM

  1. We take the users into the block editor from many different paths. As a result, we'd like to be more clear about where "back" goes to. Does it go to the pages list? Does it go to a "customer home" page that we have? We could put text by the back button here. In the same line as this, it would be helpful to be able to change the literal href of the link so that "back" doesn't just mean "the all posts wp-admin screen"

Screen Shot 2019-11-12 at 10 28 11 AM

IMO it makes sense to add extension points to the back button. These sorts of changes could be helpful to anyone implementing the block editor in a custom context where navigation doesn't happen inside of wp-admin, or does so in a more custom way (i.e. between two posts).

Our alternative to this is the following file: https://github.com/Automattic/wp-calypso/blob/master/apps/full-site-editing/full-site-editing-plugin/full-site-editing/plugins/close-button-override/index.js. As you can see, there is a high amount of fragile DOM manipulation that we'd like to avoid if possible.

@epiqueras
Copy link
Contributor

I actually see these things as problematic outside of any plugin contexts, maybe this should be the default, @youknowriad ?

@noahtallen
Copy link
Member

I actually see these things as problematic outside of any plugin contexts

I definitely agree with this :) Having the button show up all the time makes a lot of sense to me.

Though, we would still like to be able to display a label, and also override the label/href. Maybe that could be the extension point? Should that be done with a filter, or some bit of state like the features we are using here?

@epiqueras
Copy link
Contributor

Maybe an editor setting? I think we are avoiding filters where possible.

@Addison-Stavlo
Copy link
Contributor Author

If it ends up being agreeable that this icon button should always be shown, I could work on changing this from a 'fullscreen' close button to a standard close button that is always rendered. I think this would be a good option, but I was assuming there was a reason this was designed for fullscreen only and did not want to change the default behavior before opening up a discussion like this.

Similarly we could choose to always render the label depending on screen width, or keep this default as false and have this option to turn that label on.

In regards to overriding the href/label text

Maybe an editor setting? I think we are avoiding filters where possible.

For an 'editor setting' I could definitely set those optional overrides up in the store to allow for this extra control where needed.

I am open to other ideas as well. With a little more feedback on which direction we should end up taking this I would be happy to revise this and polish it up further.

@epiqueras epiqueras added the Needs Design Feedback Needs general design feedback. label Nov 12, 2019
@youknowriad
Copy link
Contributor

I agree as well that the current behavior in Core can be confusing sometimes and that we should rather fix this in Core. I don't think making every part of the UI filterable or tweakable is a good solution for these things. If folks want a completely different layout, they can build their own and avoid using the edit-post module.

I know we discussed this specific button previously at several occasions, not sure what the conclusions were. Let's bring some designers into the discussion cc @jasmussen @mapk @karmatosed

@jasmussen
Copy link
Contributor

jasmussen commented Nov 13, 2019

Thanks for the ping Riad.

Help me understand exactly what the goal is here. I'm sure it's a noble goal, I'm just trying to piece the context together. From what I gather, there is a desire to add features to the Back button so it takes you to the post type list, correct? I.e. if you're editing a Page, the back button points you to the Pages section?

Without additional context, my immediate take-away is that this is a primarily a full-screen affordance — that the back button, where it takes you, and what a label might say, should be relegated to fullscreen mode and possibly mobile, but not the non-fullscreen desktop interface.

I happen to think that fullscreen mode should also be default and non-optional, but that's a separate discussion.

Mobile makes sense, but don't underestimate that challenge. It will almost certainly require additional pull requests and design work, as there simply isn't space. Test with German on a saved document on an iPhone 5, and there's no room left. Given most browsers also add actual back buttons, I would also say that one is less urgent.

If the discussion is indeed around bringing the back button to the non-fullscreen desktop editor as well, I'm not sure what value it will bring. There's already a browser back button, there's a menu item in the navigation sidebar on the left that is highlighted, and the context seems clear. Additional back buttons would likely confuse both users and screen readers. I'm not even sure where it would take you — there isn't a "Back" button on the Settings screen either.

@noahtallen
Copy link
Member

so it takes you to the post type list

To an arbitrary page — e.g. we tell the customer on page A that they can edit their homepage, the back button should go back to A instead of the page list.

I’ll let some other folks like @shaunandrews and @apeatling respond to the rest of it :)

@jasmussen
Copy link
Contributor

jasmussen commented Nov 14, 2019

To an arbitrary page — e.g. we tell the customer on page A that they can edit their homepage, the back button should go back to A instead of the page list.

That seems totally fine in an environment that does not have a left side navigation rail.

But in an environment that does, it seems to counteract said navigation.

@noahtallen
Copy link
Member

But in an environment that does, it seems to counteract said navigation.

Indeed, that's why we'd like to add this as an optional extension point, because Gutenberg is used in environments without the left navigation rail (assuming you are talking about the wp-admin sidebar?).

PS I think the text you quoted is copied from elsewhere :)

@jasmussen
Copy link
Contributor

Indeed, that's why we'd like to add this as an optional extension point, because Gutenberg is used in environments without the left navigation rail (assuming you are talking about the wp-admin sidebar?).

Making the button toggle-able as an extension point seems also fine, though I imagine @mtias might have thoughts here — if he doesn't react to the ping it means he doesn't :D. As the block editor can increasingly be used outside of WordPress itself, even in PWA contexts, it only makes sense to have that back button.

So to be extra clear, I was referring purely to having the back button show by default in the WordPress context, when not fullscreen.

PS I think the text you quoted is copied from elsewhere :)

OH how embarrassing :D — but thankfully fun. Someone looking at the edit history can have a little chuckle there. Appreciate the heads up 👍 👍

@mtias
Copy link
Member

mtias commented Nov 15, 2019

This doesn't make a lot of sense to me as an extension point (whether preference or feature flag). Preferences and features are used for things that are meaningful to users not to support varying configurations of external environments that are not directly known and cannot be tested against.

As @youknowriad mentions, when people want to construct their own flavour of the editor UI for whatever reason (which is something that is encouraged!) the best way is to declare their own edit-post tree. Otherwise there will be an unmanageable amount of possible extensions caused by all the diverse ways people could use the editor (and their combinations) which will make iterating on the UI nearly impossible without breaking things for someone.

It might make sense to make a proposal for always showing the label on the back button when full-screen mode is engaged. Then it'd a consistent artefact of an experience that we do support and can commit to maintaining.

@chrisvanpatten
Copy link
Contributor

chrisvanpatten commented Nov 15, 2019

I think there’s a compelling argument for enforcing visible label. I’ve always found this button confusing (my brain can’t stop interpreting it as “exit fullscreen” rather than “exit editor”) and think the label text would provide a great bit of clarity.

@epiqueras
Copy link
Contributor

I think there’s a compelling argument for enforcing visible label. I’ve always found this button confusing (my brain can’t stop interpreting it as “exit fullscreen” rather than “exit editor”) and think the label text would provide a great bit of clarity.

Yes, my brain does the same.

@Addison-Stavlo Addison-Stavlo force-pushed the try/close-button-windowed-and-labeled-options branch from 4a933bb to 478cf73 Compare November 22, 2019 20:06
@Addison-Stavlo
Copy link
Contributor Author

Ok! Thank you all for your time and input regarding this discussion.

It sounds like we don't want most of the original changes that were proposed, but it seems there is a fair bit of agreement on showing the label itself.

I have reverted the entirety of the changes and updated this to only add a visible label to the current close button. There are no longer any changes to when this close button renders for screen mode or width (while it may be worth some follow up work to get this button in a state where it can be reasonably displayed on all mobile screens).

What do we think about applying a change similar to this? Below are a few screenshots of how the label is rendered in the close button. I gave the label itself 7px of right side padding to accommodate for the white space in the left half of the svg containing the arrow icon to try to keep the button centered and "balanced."

Unfocused
Screen Shot 2019-11-22 at 3 23 06 PM

Screen Shot 2019-11-22 at 3 28 57 PM

Focused
Screen Shot 2019-11-22 at 3 23 33 PM

Screen Shot 2019-11-22 at 3 29 11 PM

Inspected
Screen Shot 2019-11-22 at 3 22 31 PM

Screen Shot 2019-11-22 at 3 29 32 PM

@Addison-Stavlo Addison-Stavlo changed the title Try - add close button preferences for alwaysShow and showLabel Try - add close button visible label Nov 22, 2019
@epiqueras epiqueras requested a review from jasmussen December 3, 2019 18:18
Copy link
Contributor Author

@Addison-Stavlo Addison-Stavlo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simplified to only add label.

Class names that had requested changes are no longer present.

Copy link
Contributor

@epiqueras epiqueras left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, but I'd like to get @jasmussen to give it a final look.

Will this fit well in all screen sizes?

@Addison-Stavlo
Copy link
Contributor Author

Will this fit well in all screen sizes?

From what I have seen, yes. This no longer changes the conditions at which the button renders with screen width, so it is still not present on thin-widths.

@jasmussen
Copy link
Contributor

Thanks for the ongoing work here! Here's a fresh screenshot:

Screenshot 2019-12-05 at 10 40 54

The label is helpful in fullscreen. I wonder about the phrasing, though. Could we label it, simply, "Back"? We could add a tooltip to show more detail, "Go back to the Posts page".

There are a couple of reasons why this might make sense, the primary one being translation widths, where German adds 20% width on average. Combine that with the top toolbar and a thin viewport, and things can break in unexpected ways.

Screenshot 2019-12-05 at 10 45 36

I suspect we could probably address such issues through careful block toolbar design and top toolbar changes. But the behavior, to my understanding, is to show View [Custom Post Type]. Which means you could end up with situations where the custom post type label breaks the entire layout in ways we can't easily address without actually applying a max-width to the button, and text-overflow: ellipsis;, at which point the label itself becomes moot. Simply labelling the button, "Back", would address that, while still hopefully making the behavior more clear than it is today. Alternatively, we'll have to think about the overflow challenge.

What do you think?

@epiqueras
Copy link
Contributor

Simply labelling the button, "Back", would address that, while still hopefully making the behavior more clear than it is today.

I think this makes the most sense.

@pablohoneyhoney
Copy link

Maybe I'm still missing something so apologies, but my vote would be to not have the label by default.

  • It has with label a too prominent presence for a tertiary action.
  • View posts seems very specific to WordPress and blogging.
  • It's a UI that is been discussed in isolation of the whole environment and content that user needs to deal with. We sacrifice reduction and focus.
  • The icon's behavior is learned in a single interaction. Given its tertiary nature, not having visible label doesn't seem a hard tradeoff.
  • And I would love to understand more what was found specifically in the user tests @Addison-Stavlo that drives this proposal.

@noahtallen
Copy link
Member

Primarily, it's the use case of the new user who may not understand how navigation works in this context. At first glance, the icon can seem destructive, so they might avoid it. They also may not understand exactly where it will take them when they click it. (Both cases came up in user testing.)

a tertiary action.

What makes it tertiary? If full screen mode is the primary interaction (as it may be for different types of sites), "going back" is an action that I could see happening nearly every time the user visits the editor.

In general, I think non-power users (i.e. most people) struggle to understand the wide variety of buttons and interactions that Gutenberg exposes to them.

@pablohoneyhoney
Copy link

I agree 100% we need to think about non-power users, but a back icon is not such a novel paradigm to not understand how navigation works in this context.

The primary interaction is creating, editing, publishing content. Secondary interactions are those that support the former. Going back isn't a primary interaction in this interface, probably in any interface.

Generally, I recommend to design a good default for the majority of cases, not for first-time interactions. If you think the first-time is confusing (again, I don't think it's novel for people and it takes once to learn), there are other transitory solutions to understand that behavior and then vanish to ultimately favor a cleaner, more contextual, adaptive, and reductive interface.

@Addison-Stavlo
Copy link
Contributor Author

Addison-Stavlo commented Dec 7, 2019

Thanks @jasmussen for pointing out the horizontal overload given the german translation. While I think changing the label to "Back" in all cases is doable, it seems the IconButton does not display the on-hover tooltip label if it contains child elements. We could make a work around to both display "Back" and the on-hover View $type-of-post, but "Back" is just a bit less semantic than the label that is already being passed to the component. Perhaps just "Pages" or "Posts" as opposed to "View Pages" would be a good option if we wanted to go that route?

It seems we still have some debate on whether or not the label should be visible in the first place. At this point I am in no hurry to push this any further as we have more important issues to concentrate on for the time being. Originally we had hoped to add extension points for this button, when we decided that wasn't the best route to take I figured we would see if we wanted to keep any other changes while we were on the topic of this button. If we are not in agreement of a clear path forward, we might as well concentrate on more important issues.

To respond to @pablohoneyhoney's comments:

but a back icon is not such a novel paradigm to not understand how navigation works in this context.

It is surprising. I know its hard to believe but it does happen! When new users get into the editor, there are all sorts of new and unfamiliar buttons and icons to get used to. In some cases this can be a bit overwhelming to the point where something as simple as a < doesn't register to them as a "go back" navigation icon. While they would eventually figure it out in time, it becomes another stumbling point in their first experience.

These stumbling points add up, and it is often the case that the user seems to be getting a bit "fried" on learning the new interface and needs to take a break from editing their site before they have even really begun to do so. This leaves their first WordPress editing experience fairly unproductive, and by reducing these stumbling points and making their first interaction as simple and understandable as possible we can start to move in a direction where users can get more milage in their first interactions with the editor. In the end it is "just a back button," but every little bit helps.

I recommend to design a good default for the majority of cases, not for first-time interactions.

Can't it be both? If adding something will help first-time interactions while not detracting from the majority, we should seek to do so. If we continue to design the interface for use by people that will already know their way around the interface, while carrying the assumption that new users "will figure it out eventually," we will end up limiting the reach and influence of the product. We should consider that a lot of people who want to build a website are new to the process and just need to build one "simple" website. And in the process of deciding on which tool to use and carrying it out, the majority of their interactions are first-time interactions.

The primary interaction is creating, editing, publishing content. Secondary interactions are those that support the former. Going back isn't a primary interaction in this interface, probably in any interface.

I would like to argue that "back" navigation is a very important interaction in a web app these days. There are handfuls of poorly designed single-page component flows out there where hitting the browser's default "back" button may take a user much further back than they expected to go. I know I have experienced these on government or corporate sites in filling out information or going through other tasks where I have hit the browsers "back" button and it has taken me not to the last page I was viewing, but to the first page of the user flow itself.

While we strive to design better than this, the fact that these sites exist means that new users have experienced them. This means they can come in with a bias towards being afraid of using the browsers "back" button, while having one embedded into the web app itself is generally always seen as a "safe" option. This is also why I believe we need to consider adding the < button in mobile viewports.

@epiqueras
Copy link
Contributor

I think there’s a compelling argument for enforcing visible label. I’ve always found this button confusing (my brain can’t stop interpreting it as “exit fullscreen” rather than “exit editor”) and think the label text would provide a great bit of clarity.

This is my only issue with the current state of the button.

Now I think that the most progressive way to fix it is to get rid of the button altogether and rely on the browser's back button. It does the same thing, why not avoid the potential confusion?

@chrisvanpatten
Copy link
Contributor

Now I think that the most progressive way to fix it is to get rid of the button altogether and rely on the browser's back button. It does the same thing, why not avoid the potential confusion?

Agreed. Why reinvent the wheel here? What value does this button provide that the native browser “Back” button can’t provide?

@Addison-Stavlo
Copy link
Contributor Author

I am going to close this PR. Thank you all for your time and discussion on this issue.

Now I think that the most progressive way to fix it is to get rid of the button altogether and rely on the browser's back button.

While I would like to see a back button around, that may end up being the best thing to do. Especially if real-estate on that bar continues to become more necessary for other things.

It does the same thing, why not avoid the potential confusion?

I think the only potential confusion by not having it would be initial (based on what I was saying in the last response on my latest post above). If a new user had any concerns about the browser's back button potentially taking them to a non-desired location, they would learn that it is safe and works as intended once they just try it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Extensibility The ability to extend blocks or the editing experience Needs Design Feedback Needs general design feedback. [Package] Edit Post /packages/edit-post
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants