-
Notifications
You must be signed in to change notification settings - Fork 4.5k
Try - add close button visible label #18387
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Try - add close button visible label #18387
Conversation
There was a problem hiding this 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 ) { |
There was a problem hiding this comment.
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.
packages/edit-post/src/components/header/fullscreen-mode-close/index.js
Outdated
Show resolved
Hide resolved
cc @epiqueras @youknowriad who would be a good person to give some feedback on these changes? |
There was a problem hiding this 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"?
packages/edit-post/src/components/header/fullscreen-mode-close/index.js
Outdated
Show resolved
Hide resolved
@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? |
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.
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. |
I actually see these things as problematic outside of any plugin contexts, maybe this should be the default, @youknowriad ? |
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? |
Maybe an editor setting? I think we are avoiding filters where possible. |
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
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. |
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 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 |
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. |
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 :) |
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. |
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 :) |
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.
OH how embarrassing :D — but thankfully fun. Someone looking at the edit history can have a little chuckle there. Appreciate the heads up 👍 👍 |
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 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. |
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. |
4a933bb
to
478cf73
Compare
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." |
There was a problem hiding this 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.
There was a problem hiding this 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?
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. |
Thanks for the ongoing work here! Here's a fresh screenshot: 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. 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 What do you think? |
I think this makes the most sense. |
Maybe I'm still missing something so apologies, but my vote would be to not have the label by default.
|
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.)
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. |
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. |
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 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:
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 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.
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.
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 |
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? |
Agreed. Why reinvent the wheel here? What value does this button provide that the native browser “Back” button can’t provide? |
I am going to close this PR. Thank you all for your time and discussion on this issue.
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.
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. |
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 theFullscreenModeClose
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?
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 ).
wp.data.dispatch( 'core/edit-post' ).toggleFeature( 'alwaysShowCloseButton' )
a. editor is in windowed mode.
b. screen width is below medium width ( 782px ).
wp.data.dispatch( 'core/edit-post' ).toggleFeature( 'showCloseButtonLabel' )
Screenshots
Initial Windowed View

alwaysShow toggled on in windowed mode

alwaysShow toggled on in fullscren mobile width

showLabel toggled on in windowed mode

Types of changes
Checklist: