Skip to content
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 page name to document settings [IN PROGRESS] #25386

Closed
wants to merge 13 commits into from
1 change: 1 addition & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions packages/edit-site/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@
"@wordpress/plugins": "file:../plugins",
"@wordpress/primitives": "file:../primitives",
"@wordpress/url": "file:../url",
"classnames": "^2.2.5",
"downloadjs": "^1.4.7",
"file-saver": "^2.0.2",
"jszip": "^3.2.2",
Expand Down
152 changes: 152 additions & 0 deletions packages/edit-site/src/components/header/document-actions/index.js
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>
Comment on lines +86 to +102
Copy link
Member

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 label attribute 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 the label attribute right now.

Also, you might be able to shorten the label to just "Document settings".

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Sep 16, 2020

Choose a reason for hiding this comment

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

Additionally, it is an accessibility anti-pattern to use current state to label a button. The visible label should just be what's in the label attribute right now.

An aria-label should override any button text regarding accessibility. The visible text should be fine as a basic name as long as we aria-label with the action?

Copy link
Member

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.

While the efforts towards accessibility have been significant, there has been a strong dependence on methods we consider to be anti-patterns. First among these is a heavy use of ARIA attributes. ARIA, while sometimes necessary, should be considered only as a final option when no native HTML interactions are available.

This reflects a problem in the design process: beginning with custom controls (such as the “switch toggle”), then attempting to make them accessible rather than beginning with standard controls and making a functional interface.

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

An aria-label should override any button text regarding accessibility. The visible text should be fine as a basic name as long as we aria-label with the action?

@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.

Copy link
Contributor

@Addison-Stavlo Addison-Stavlo Sep 17, 2020

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 Part names side by side and need to stack them vertically, and something likeOpen Template: ${template_name}'s settings or 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)

Copy link
Member

Choose a reason for hiding this comment

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

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 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.

Copy link
Member

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:

Inside template part: Header  |  Selected block: Paragraph  |  Visibility: Public  |  Publish date: Dec 12, 2020

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):

Maybe we ought to consider adding a "document status/overview" popover/modal/something that lists out important details of the document (e.g. visibility, publish state, SEO status), with "Edit" links to jump to the parts of the UI dedicated to changing those details. Maybe we could even combine it with the "Content structure" tool (the one that shows word count and stuff like that)? Just throwing ideas out there.

Copy link
Contributor

Choose a reason for hiding this comment

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

And I don't think it belongs in the top bar...

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @Addison-Stavlo !

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.

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.

);
} }
renderContent={ () => (
<div
Copy link
Member

Choose a reason for hiding this comment

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

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)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Totally agreed 👍

style={ {
display: 'flex',
justifyContent: 'space-between',
alignItems: 'center',
} }
>
{ /* TODO: Replace inline styles */ }
<span style={ { marginRight: '12px' } }>
Name
</span>
Comment on lines +114 to +116
Copy link
Member

Choose a reason for hiding this comment

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

This should be a <label> associated via the for prop with the <input> that follows it. You can use useInstanceId to help make a unique id for the <input>.

Copy link
Contributor Author

@jeyip jeyip Sep 21, 2020

Choose a reason for hiding this comment

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

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>
);
}
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%;
}
}
}
125 changes: 71 additions & 54 deletions packages/edit-site/src/components/header/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ import SaveButton from '../save-button';
import UndoButton from './undo-redo/undo';
import RedoButton from './undo-redo/redo';
import FullscreenModeClose from './fullscreen-mode-close';
import DocumentActions from './document-actions';

export default function Header( {
openEntitiesSavedStates,
Expand All @@ -36,6 +37,7 @@ export default function Header( {
const {
deviceType,
hasFixedToolbar,
template,
templateId,
templatePartId,
templateType,
Expand All @@ -51,14 +53,18 @@ export default function Header( {
getPage,
} = select( 'core/edit-site' );

const { show_on_front: _showOnFront } = select(
'core'
).getEditedEntityRecord( 'root', 'site' );
const { getEntityRecord, getEditedEntityRecord } = select( 'core' );
const { show_on_front: _showOnFront } = getEditedEntityRecord(
'root',
'site'
);

const _templateId = getTemplateId();
return {
deviceType: __experimentalGetPreviewDeviceType(),
hasFixedToolbar: isFeatureActive( 'fixedToolbar' ),
templateId: getTemplateId(),
templateId: _templateId,
template: getEntityRecord( 'postType', 'wp_template', _templateId ),
templatePartId: getTemplatePartId(),
templateType: getTemplateType(),
page: getPage(),
Expand All @@ -81,62 +87,73 @@ export default function Header( {

return (
<div className="edit-site-header">
<MainDashboardButton.Slot>
<FullscreenModeClose />
</MainDashboardButton.Slot>
<div className="edit-site-header__toolbar">
<Button
isPrimary
isPressed={ isInserterOpen }
className="edit-site-header-toolbar__inserter-toggle"
onClick={ onToggleInserter }
icon={ plus }
label={ _x(
'Add block',
'Generic label for block inserter button'
<div className="edit-site-header_start">
<MainDashboardButton.Slot>
<FullscreenModeClose />
</MainDashboardButton.Slot>
<div className="edit-site-header__toolbar">
<Button
isPrimary
isPressed={ isInserterOpen }
className="edit-site-header-toolbar__inserter-toggle"
onClick={ onToggleInserter }
icon={ plus }
label={ _x(
'Add block',
'Generic label for block inserter button'
) }
/>
<ToolSelector />
<UndoButton />
<RedoButton />
<BlockNavigationDropdown />
{ displayBlockToolbar && (
<div className="edit-site-header-toolbar__block-toolbar">
<BlockToolbar hideDragHandle />
</div>
) }
/>
<ToolSelector />
<UndoButton />
<RedoButton />
<BlockNavigationDropdown />
{ displayBlockToolbar && (
<div className="edit-site-header-toolbar__block-toolbar">
<BlockToolbar hideDragHandle />
<div className="edit-site-header__toolbar-switchers">
<PageSwitcher
showOnFront={ showOnFront }
activePage={ page }
onActivePageChange={ setPage }
/>
<div className="edit-site-header__toolbar-switchers-separator">
/
</div>
<TemplateSwitcher
page={ page }
activeId={ templateId }
activeTemplatePartId={ templatePartId }
isTemplatePart={
templateType === 'wp_template_part'
}
onActiveIdChange={ setTemplate }
onActiveTemplatePartIdChange={ setTemplatePart }
onAddTemplate={ addTemplate }
onRemoveTemplate={ removeTemplate }
/>
</div>
) }
<div className="edit-site-header__toolbar-switchers">
<PageSwitcher
showOnFront={ showOnFront }
activePage={ page }
onActivePageChange={ setPage }
</div>
</div>

<div className="edit-site-header_center">
<DocumentActions documentTitle={ template?.slug } />
</div>

<div className="edit-site-header_end">
<div className="edit-site-header__actions">
<PreviewOptions
deviceType={ deviceType }
setDeviceType={ setPreviewDeviceType }
/>
<div className="edit-site-header__toolbar-switchers-separator">
/
</div>
<TemplateSwitcher
page={ page }
activeId={ templateId }
activeTemplatePartId={ templatePartId }
isTemplatePart={ templateType === 'wp_template_part' }
onActiveIdChange={ setTemplate }
onActiveTemplatePartIdChange={ setTemplatePart }
onAddTemplate={ addTemplate }
onRemoveTemplate={ removeTemplate }
<SaveButton
openEntitiesSavedStates={ openEntitiesSavedStates }
/>
<PinnedItems.Slot scope="core/edit-site" />
<MoreMenu />
</div>
</div>
<div className="edit-site-header__actions">
<PreviewOptions
deviceType={ deviceType }
setDeviceType={ setPreviewDeviceType }
/>
<SaveButton
openEntitiesSavedStates={ openEntitiesSavedStates }
/>
<PinnedItems.Slot scope="core/edit-site" />
<MoreMenu />
</div>
</div>
);
}
17 changes: 15 additions & 2 deletions packages/edit-site/src/components/header/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -2,13 +2,26 @@
align-items: center;
display: flex;
height: $header-height;
justify-content: space-between;
box-sizing: border-box;

.edit-site-header_start,
.edit-site-header_end {
flex: 1;
display: flex;
}

.edit-site-header_center {
display: flex;
height: 100%;
}

.edit-site-header_end {
justify-content: flex-end;
}
}

.edit-site-header__toolbar {
display: flex;
flex-grow: 1;
padding-left: $grid-unit-30;
align-items: center;

Expand Down
Loading