Try/add page name to document settings [IN PROGRESS] #25386
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,152 @@ | ||
| /** | ||
| * External dependencies | ||
| */ | ||
| import classnames from 'classnames'; | ||
|
|
||
| /** | ||
| * WordPress dependencies | ||
| */ | ||
| import { Button, Dropdown } from '@wordpress/components'; | ||
| import { __ } from '@wordpress/i18n'; | ||
| import { | ||
| __experimentalGetBlockLabel as getBlockLabel, | ||
| getBlockType, | ||
| } from '@wordpress/blocks'; | ||
| import { useDispatch, useSelect } from '@wordpress/data'; | ||
| import { DOWN } from '@wordpress/keycodes'; | ||
|
|
||
| function useSecondaryText() { | ||
| const selectedBlock = useSelect( ( select ) => { | ||
| return select( 'core/block-editor' ).getSelectedBlock(); | ||
| } ); | ||
|
|
||
| // TODO: Handle if parent is template part too. | ||
| const selectedBlockLabel = | ||
| selectedBlock?.name === 'core/template-part' | ||
| ? getBlockLabel( | ||
| getBlockType( selectedBlock?.name ), | ||
| selectedBlock?.attributes | ||
| ) | ||
| : null; | ||
|
|
||
| if ( selectedBlockLabel ) { | ||
| return { | ||
| label: selectedBlockLabel, | ||
| isActive: true, | ||
| }; | ||
| } | ||
| return {}; | ||
| } | ||
|
|
||
| export default function DocumentActions( { documentTitle } ) { | ||
| const { label, isActive } = useSecondaryText(); | ||
| // Title is active when there is no secondary item, or when the secondary | ||
| // item is inactive. | ||
| const isTitleActive = ! label?.length || ! isActive; | ||
|
|
||
| const { page } = useSelect( ( select ) => { | ||
| const { getPage } = select( 'core/edit-site' ); | ||
| const { getEntityRecord } = select( 'core' ); | ||
| const _page = getPage(); | ||
| return { | ||
| page: getEntityRecord( 'postType', 'page', _page?.context?.postId ), | ||
| }; | ||
| } ); | ||
|
|
||
| const { editEntityRecord } = useDispatch( 'core' ); | ||
| const editTitle = ( title ) => { | ||
| editEntityRecord( 'postType', 'page', page.id, { | ||
| title, | ||
| slug: title, | ||
| } ); | ||
| }; | ||
|
|
||
| return ( | ||
| <div | ||
| className={ classnames( 'edit-site-document-actions', { | ||
| 'has-secondary-label': !! label, | ||
| } ) } | ||
| > | ||
| { documentTitle ? ( | ||
| <> | ||
| <Dropdown | ||
| position="bottom center" | ||
| renderToggle={ ( { onToggle, isOpen } ) => { | ||
| const openOnArrowDown = ( event ) => { | ||
| if ( ! isOpen && event.keyCode === DOWN ) { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| onToggle(); | ||
| } | ||
| }; | ||
| { | ||
| /* TODO: Fix vertical text padding */ | ||
| } | ||
| return ( | ||
| <Button | ||
| onClick={ onToggle } | ||
| className={ classnames( | ||
| 'edit-site-document-actions__label', | ||
| 'edit-site-document-actions__title', | ||
| { | ||
| 'is-active': isTitleActive, | ||
| } | ||
| ) } | ||
| aria-haspopup="true" | ||
| aria-expanded={ isOpen } | ||
| onKeyDown={ openOnArrowDown } | ||
| label={ __( 'Change document settings.' ) } | ||
| showTooltip | ||
| > | ||
| { documentTitle } | ||
| </Button> | ||
| ); | ||
| } } | ||
| renderContent={ () => ( | ||
| <div | ||
|
There was a problem hiding this comment. I think it would be helpful to split out the actual settings area into a separate component, even at this early stage :) (including the page selector above) |
||
| style={ { | ||
| display: 'flex', | ||
| justifyContent: 'space-between', | ||
| alignItems: 'center', | ||
| } } | ||
| > | ||
| { /* TODO: Replace inline styles */ } | ||
| <span style={ { marginRight: '12px' } }> | ||
| Name | ||
| </span> | ||
|
Comment on lines
+114
to
+116
There was a problem hiding this comment. This should be a There was a problem hiding this comment. Thanks @ZebulanStanphill I'm still working on the main functionality of the PR, but I'll be sure to circle back and address this too before the next cycle of code review 👍 |
||
| { /* TODO: Don't allow input when there is no page context */ } | ||
| <input | ||
| disabled={ ! page } | ||
| placeholder={ page?.title?.rendered } | ||
| style={ { width: '100%' } } | ||
| onChange={ ( event ) => { | ||
| const REGEXP_NEWLINES = /[\r\n]+/g; | ||
| const title = event.target.value.replace( | ||
| REGEXP_NEWLINES, | ||
| ' ' | ||
| ); | ||
|
|
||
| editTitle( title ); | ||
| } } | ||
| /> | ||
| </div> | ||
| ) } | ||
| ></Dropdown> | ||
| <div | ||
| className={ classnames( | ||
| 'edit-site-document-actions__label', | ||
| 'edit-site-document-actions__secondary-item', | ||
| { | ||
| 'is-active': isActive, | ||
| } | ||
| ) } | ||
| > | ||
| { label ?? '' } | ||
| </div> | ||
| </> | ||
| ) : ( | ||
| __( 'Loading…' ) | ||
| ) } | ||
| </div> | ||
| ); | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,34 @@ | ||
| .edit-site-document-actions { | ||
| display: flex; | ||
| flex-direction: column; | ||
| align-items: center; | ||
| justify-content: space-evenly; | ||
|
|
||
| .edit-site-document-actions__label { | ||
| color: $gray-700; | ||
| display: flex; | ||
| justify-content: center; | ||
| align-items: center; | ||
| transition: height 0.25s; | ||
|
|
||
| &.is-active { | ||
| color: inherit; | ||
| } | ||
|
|
||
| &.edit-site-document-actions__title { | ||
| height: 100%; | ||
| // Otherwise, the secondary item still takes up space with height 0: | ||
| flex-grow: 1; | ||
| } | ||
|
|
||
| &.edit-site-document-actions__secondary-item { | ||
| height: 0; | ||
| } | ||
| } | ||
|
|
||
| &.has-secondary-label { | ||
| .edit-site-document-actions__label { | ||
| height: 50%; | ||
| } | ||
| } | ||
| } |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
I don't think the
labelattribute works if you also have a nested text label. Additionally, it is an accessibility anti-pattern (i.e. something you should avoid) to use current state to label a button. The visible label should just be what's in thelabelattribute right now.Also, you might be able to shorten the label to just "Document settings".
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.
An
aria-labelshould override any button text regarding accessibility. The visible text should be fine as a basic name as long as wearia-labelwith the action?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.
ARIA attributes are supposed to be a last resort, not a band-aid to let one give any visible label to a button and call it accessible. A
<button>should, if at all possible, use a visible text label that actually functions as a label. Accessibility doesn't just apply to screen readers; it applies to all methods of interaction.https://make.wordpress.org/accessibility/2018/10/29/report-on-the-accessibility-status-of-gutenberg/
The accessibility team has wanted issues like #470 to be fixed for over 3 years now. Only now is it looking like that particular issue will be resolved. (See #25170 and #24024.) We shouldn't be introducing more buttons that have the same problem but even less of a reason to not use a proper label.
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.
@Addison-Stavlo as I just explained on #25085 (comment) a mismatch between the visible text and the aria-label is a huge barrier for some users and violates WCAG Label in Name.
Also, totally agree with @ZebulanStanphill that ARIA should be used as last resort and also in that case should be used appropriately. Always prefer meaningful visible text, please.
Also, since the underlying issue in #470 is being discussed since more than 3 years now, I'd expect designers and developers in this project to be aware of that and to share information when new designers / developers join the project, otherwise repeating over and over the same recommendation is going to be unsustainable for the accessibility team. I do realize this part is more related to process but, alas, it appears the process has big room for improvements.
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.
Thanks for the insight, and thank you for the explanation there on that issue @afercia. It is definitely helpful.
I think a large barrier we run into that makes naming buttons after their actions difficult is the available real estate in the areas they need to go. For example in the available room in this top bar can be extremely limited in tablet/mobile views as well as with the top-toolbar mode activated. So much so we can't afford to put
Template / Template Partnames side by side and need to stack them vertically, and something likeOpen Template: ${template_name}'s settingsor something similar would not be an option here either. With limited room, how do we go about adding more interactions while still making them accessible to all accessibility flows? 🤔 (I don't expect an explicit answer, this is just something I am trying to ponder as I learn more about accessibility)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.
I totally get the frustration here. :) I think this probably applies to so many things across the block editor. For example, a specific data store might cache data in a very specific way for performance reasons, but a new developer to the project won't understand that context. So they could easily make a change which circumvents that caching, causing a performance regression. (That's me, I made a change like that!)
I don't think there is really any sort of onboarding process at all for new devs, other than reading the documentation and code (which is fairly poor, unfortunately.) I wonder if there is a sustainable way to share this information, since I imagine there are a lot of similar examples. 🤔 Because that would definitely be great for both you and me in this situation! I think at least partially, some of these issues are just an ongoing learning process for everyone.
Part of that is Gutenberg contributions are really accessible to a broad range of folks: it doesn't require much of anything to start trying to contribute. So there are lots of contributors with varying ranges of experience on different topics, which means we don't really have that shared base of common skills or knowledge. :/ Which goes back into what you said: would it be helpful to create a way to generate a baseline of knowledge for new contributors? ANYWays, we're getting pretty meta here. :p
I think the space issue @Addison-Stavlo mentions is going back to the root designs we are working with here. I know that y'all have brought up this topic tons of times in the discussions about these designs. Point being, it'd be really nice to all get on the same page about how exactly this is going to work, rather than splitting this discussion across several issues.
And part of that being, we're exploring and creating some prototypes for what these might look like, so I don't think we have to treat this PR as if we are trying to do something outside of the discussion so far. We're very happy to incorporate improvements that we reach consensus on! I think in this case, it is difficult as a dev to incorporate improvements if we aren't completely sure what this interaction should look like after all of the suggestions are in place. 🤔 I basically don't want to get into a place where we implement X, and which you disagree on, so we implement Y, which design disagrees on, etc.
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.
I think part of the problem here is that you're trying to conflate two things that should be separate: an indicator of the current template title, and a button to change the template's title/url/etc. Trying to have one UI element serve as both probably won't work.
The purpose of the template settings button is to open an interface to edit the template settings. So it should be labeled accordingly, and it's position in the UI would logically be somewhere in the top bar.
The indicator of the current template's title, however, is something different. And I don't think it belongs in the top bar... at least not in the same row as everything else. It's already weird that there's a left half and a right half to the top bar. I think adding a middle section would just confuse the layout even more.
I think what we're looking for is a sort of "status bar". Something that is designed to present information to the user first and foremost, with any interactivity being a "nice to have" that isn't treated as the primary way to do anything.
Maybe this bar could look something like this:
Perhaps this status bar could be placed below the top bar or maybe even above it. Maybe it could take the place of the current footer bar? I'm not sure.
I also had another idea for presenting "status" info, which I'll quote from #25353 (comment):
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.
I think you have some good ideas, but I will note that #25085 specifically for exposing these names in the top bar has been generally agreed upon and added to the next phase of the Site Editors infrastructure development #24818 . We are just moving forward with that infrastructure and UI development agenda.
I do welcome your concerns and ideas, however this PR may not be the best place to raise concerns with whether or not we should show the names here or in a new status bar, as most of the eyes involved in that consensus and decision may not see them. Nothing is final and even if we (together as core) move forward to put the labels in this region, we are still able to discuss better solutions and reiterate. But for now, there needs to be a starting point to assess and start iterating from and putting them here seems to be the current consensus for that starting point.
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.
Thanks @Addison-Stavlo !
Well, generally any UI should be designed around the content, not the other way around 🙂 Trying to "squeeze" controls and text into a limited space is a clear sign the UI has some problems in the first place. I do realize this feedback doesn't propose an alternative solution. Maybe, some lateral thinking could help and i fit was up to me I'd ask myself whether the top bar is really the right place for these control. Giving all the problems we're facing, I'd say it is not.