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.

Sign up for GitHub

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

Jump to bottom

DocumentBar: replace icon with post type label #65170

Merged
merged 11 commits into from
Sep 17, 2024
Merged

DocumentBar: replace icon with post type label #65170

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
4e8946d
Adds post type label to DocumentBar
creativecoder Sep 9, 2024
adf84da
Removes the post type icon from the DocumentBar
creativecoder Sep 9, 2024
810435f
Try fixing one of the e2e tests
andrewserong Sep 10, 2024
58522d7
Remove IconType import and add typedef
creativecoder Sep 10, 2024
a970416
Adds font-weight to heading and uses darker grey for constrast
creativecoder Sep 10, 2024
95ed714
Adjusts title overflow so it dosen't apply to post type label
creativecoder Sep 10, 2024
539f947
Moves font weight to h1
creativecoder Sep 10, 2024
3e15324
Try fixing more e2e tests
andrewserong Sep 12, 2024
257f1fd
Fix another e2e
andrewserong Sep 12, 2024
b01b483
Add fix for mid screensizes when Back button is present
andrewserong Sep 13, 2024
6f9ae9a
Hide post type label on small screen sizes
andrewserong Sep 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 7 additions & 18 deletions packages/edit-site/src/components/editor-canvas-container/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
import { ESCAPE } from '@wordpress/keycodes';
import { __ } from '@wordpress/i18n';
import { useDispatch, useSelect } from '@wordpress/data';
import { backup, closeSmall, seen } from '@wordpress/icons';
import { closeSmall } from '@wordpress/icons';
import { useFocusOnMount, useFocusReturn } from '@wordpress/compose';
import { store as preferencesStore } from '@wordpress/preferences';
import {
Expand All @@ -32,24 +32,15 @@ const { EditorContentSlotFill, ResizableEditor } = unlock( editorPrivateApis );
*
* @return {Object} Translated string for the view title and associated icon, both defaulting to ''.
*/
function getEditorCanvasContainerTitleAndIcon( view ) {
function getEditorCanvasContainerTitle( view ) {
switch ( view ) {
case 'style-book':
return {
title: __( 'Style Book' ),
icon: seen,
};
return __( 'Style Book' );
case 'global-styles-revisions':
case 'global-styles-revisions:style-book':
return {
title: __( 'Style Revisions' ),
icon: backup,
};
return __( 'Style Revisions' );
default:
return {
title: '',
icon: '',
};
return '';
}
}

Expand Down Expand Up @@ -118,9 +109,7 @@ function EditorCanvasContainer( {
return null;
}

const { title } = getEditorCanvasContainerTitleAndIcon(
editorCanvasContainerView
);
const title = getEditorCanvasContainerTitle( editorCanvasContainerView );
const shouldShowCloseButton = onClose || closeButtonLabel;

return (
Expand Down Expand Up @@ -158,4 +147,4 @@ function useHasEditorCanvasContainer() {
}

export default EditorCanvasContainer;
export { useHasEditorCanvasContainer, getEditorCanvasContainerTitleAndIcon };
export { useHasEditorCanvasContainer, getEditorCanvasContainerTitle };
6 changes: 2 additions & 4 deletions packages/edit-site/src/components/editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import PluginTemplateSettingPanel from '../plugin-template-setting-panel';
import GlobalStylesSidebar from '../global-styles-sidebar';
import { isPreviewingTheme } from '../../utils/is-previewing-theme';
import {
getEditorCanvasContainerTitleAndIcon,
getEditorCanvasContainerTitle,
useHasEditorCanvasContainer,
} from '../editor-canvas-container';
import SaveButton from '../save-button';
Expand Down Expand Up @@ -204,8 +204,7 @@ export default function EditSiteEditor( { isPostsList = false } ) {
);

// Replace the title and icon displayed in the DocumentBar when there's an overlay visible.
const { title, icon } =
getEditorCanvasContainerTitleAndIcon( editorCanvasView );
const title = getEditorCanvasContainerTitle( editorCanvasView );

const isReady = ! isLoading;
const transition = {
Expand Down Expand Up @@ -238,7 +237,6 @@ export default function EditSiteEditor( { isPostsList = false } ) {
customSavePanel={ _isPreviewingTheme && <SavePanel /> }
forceDisableBlockTools={ ! hasDefaultEditorCanvasView }
title={ title }
icon={ icon }
iframeProps={ iframeProps }
onActionPerformed={ onActionPerformed }
extraSidebarPanels={
Expand Down
2 changes: 1 addition & 1 deletion packages/editor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -266,7 +266,7 @@ _Parameters_

- _props_ `Object`: The component props.
- _props.title_ `string`: A title for the document, defaulting to the document or template title currently being edited.
- _props.icon_ `import("@wordpress/components").IconType`: An icon for the document, defaulting to an icon for document or template currently being edited.
- _props.icon_ `IconType`: An icon for the document, no default. (A default icon indicating the document post type is no longer used.)

_Returns_

Expand Down
70 changes: 29 additions & 41 deletions packages/editor/src/components/document-bar/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import clsx from 'clsx';
/**
* WordPress dependencies
*/
import { __, isRTL, sprintf } from '@wordpress/i18n';
import { __, isRTL } from '@wordpress/i18n';
import { useSelect, useDispatch } from '@wordpress/data';
import {
Button,
Expand All @@ -28,18 +28,8 @@ import { decodeEntities } from '@wordpress/html-entities';
*/
import { TEMPLATE_POST_TYPES, GLOBAL_POST_TYPES } from '../../store/constants';
import { store as editorStore } from '../../store';
import { unlock } from '../../lock-unlock';

const TYPE_LABELS = {
// translators: 1: Pattern title.
wp_pattern: __( 'Editing pattern: %s' ),
// translators: 1: Navigation menu title.
wp_navigation: __( 'Editing navigation menu: %s' ),
// translators: 1: Template title.
wp_template: __( 'Editing template: %s' ),
// translators: 1: Template part title.
wp_template_part: __( 'Editing template part: %s' ),
};
/** @typedef {import("@wordpress/components").IconType} IconType */

const MotionButton = motion( Button );

Expand All @@ -52,21 +42,21 @@ const MotionButton = motion( Button );
* ```jsx
* <DocumentBar />
* ```
* @param {Object} props The component props.
* @param {string} props.title A title for the document, defaulting to the document or
* template title currently being edited.
* @param {import("@wordpress/components").IconType} props.icon An icon for the document, defaulting to an icon for document
creativecoder marked this conversation as resolved.
Show resolved Hide resolved
* or template currently being edited.
* @param {Object} props The component props.
* @param {string} props.title A title for the document, defaulting to the document or
* template title currently being edited.
* @param {IconType} props.icon An icon for the document, no default.
* (A default icon indicating the document post type is no longer used.)
*
* @return {JSX.Element} The rendered DocumentBar component.
*/
export default function DocumentBar( props ) {
const {
postType,
postTypeLabel,
documentTitle,
isNotFound,
isUnsyncedPattern,
templateIcon,
templateTitle,
onNavigateToPreviousEntityRecord,
} = useSelect( ( select ) => {
Expand All @@ -76,8 +66,11 @@ export default function DocumentBar( props ) {
getEditorSettings,
__experimentalGetTemplateInfo: getTemplateInfo,
} = select( editorStore );
const { getEditedEntityRecord, isResolving: isResolvingSelector } =
select( coreStore );
const {
getEditedEntityRecord,
getPostType,
isResolving: isResolvingSelector,
} = select( coreStore );
const _postType = getCurrentPostType();
const _postId = getCurrentPostId();
const _document = getEditedEntityRecord(
Expand All @@ -86,8 +79,11 @@ export default function DocumentBar( props ) {
_postId
);
const _templateInfo = getTemplateInfo( _document );
const _postTypeLabel = getPostType( _postType )?.labels?.singular_name;

return {
postType: _postType,
postTypeLabel: _postTypeLabel,
documentTitle: _document.title,
isNotFound:
! _document &&
Expand All @@ -98,12 +94,6 @@ export default function DocumentBar( props ) {
_postId
),
isUnsyncedPattern: _document?.wp_pattern_sync_status === 'unsynced',
templateIcon: unlock( select( editorStore ) ).getPostIcon(
_postType,
{
area: _document?.area,
}
),
templateTitle: _templateInfo.title,
onNavigateToPreviousEntityRecord:
getEditorSettings().onNavigateToPreviousEntityRecord,
Expand All @@ -118,7 +108,7 @@ export default function DocumentBar( props ) {
const hasBackButton = !! onNavigateToPreviousEntityRecord;
const entityTitle = isTemplate ? templateTitle : documentTitle;
const title = props.title || entityTitle;
const icon = props.icon || templateIcon;
const icon = props.icon;

const mountedRef = useRef( false );
useEffect( () => {
Expand Down Expand Up @@ -187,20 +177,18 @@ export default function DocumentBar( props ) {
isReducedMotion ? { duration: 0 } : undefined
}
>
<BlockIcon icon={ icon } />
<Text
size="body"
as="h1"
aria-label={
! props.title && TYPE_LABELS[ postType ]
? // eslint-disable-next-line @wordpress/valid-sprintf
sprintf( TYPE_LABELS[ postType ], title )
: undefined
}
>
Comment on lines -194 to -200
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The aria-label doesn't seem necessary any more, since the post type is now displayed after the document title.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed that this removal causes some of the e2e tests to fail. E.g. this one:

gutenberg/test/e2e/specs/editor/various/post-editor-template-mode.spec.js

Line 40 in 50f5c74

test( 'Allow to switch to template mode, edit the template and check the result', async ( {

The failure appears to be the check on this line:

gutenberg/test/e2e/specs/editor/various/post-editor-template-mode.spec.js

Lines 198 to 200 in 50f5c74

const title = this.editorTopBar.getByRole( 'heading', {
name: 'Editing template: Single Entries',
} );

The aria-label does add a bit more context as it explains the context (editing) rather than using the visual separator. So I wonder if it's worth still including it? Either that, or updating the test 🙂

I'll just ping @jeryj on this one, too, as I remember there being previous discussions about the accessibility of this bar.

Copy link
Contributor

@jeryj jeryj Sep 10, 2024 •
edited by andrewserong
Loading

Choose a reason for hiding this comment

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

I don't think this impacts it one way or another. The main issue with the accessibility of this area is the semantics and inconsistent naming: #51394.

  1. Buttons should have consistent descriptive names, so this button needs to be visually and consistently named as "Command Palette" or something similar.
  2. The <h1> is nested within the <button> element. <button> elements often strip the semantics of its contents, so the <h1> would never be exposed. It is as if there is no heading on the page at all.

So, those core issues do need to be fixed, but this PR doesn't seem to impact the core issues either way.

{ title
? decodeEntities( title )
: __( 'No title' ) }
{ icon && <BlockIcon icon={ icon } /> }
<Text size="body" as="h1">
<span className="editor-document-bar__post-title">
{ title
? decodeEntities( title )
: __( 'No title' ) }
</span>
{ postTypeLabel && ! props.title && (
<span className="editor-document-bar__post-type-label">
{ '· ' + decodeEntities( postTypeLabel ) }
</span>
) }
</Text>
</motion.div>
<span className="editor-document-bar__shortcut">
Expand Down
48 changes: 33 additions & 15 deletions packages/editor/src/components/document-bar/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,14 @@
background: $gray-200;
}
}

&.has-back-button {
@media screen and (min-width: #{ ($break-medium) }) and (max-width: #{ ($break-large) }) {
.editor-document-bar__post-type-label {
display: none;
}
}
}
}

.editor-document-bar__command {
Expand All @@ -34,34 +42,44 @@
}

.editor-document-bar__title {
flex: 1;
overflow: hidden;
color: $gray-900;
gap: $grid-unit-05;
display: flex;
justify-content: center;
align-items: center;
margin: 0 auto;
max-width: 70%;

// Offset the layout based on the width of the ⌘K label. This ensures the title is centrally aligned.
@include break-medium() {
padding-left: $grid-unit-30;
}

h1 {
display: flex;
align-items: center;
justify-content: center;
font-weight: 400;
white-space: nowrap;
overflow: hidden;
}
}

.editor-document-bar__post-title {
color: currentColor;
flex: 1;
overflow: hidden;
text-overflow: ellipsis;

.editor-document-bar.is-global & {
color: var(--wp-block-synced-color);
}
}

.block-editor-block-icon {
min-width: $grid-unit-30;
flex-shrink: 0;
}
.editor-document-bar__post-type-label {
flex: 0;
color: $gray-800;
padding-left: $grid-unit-05;

h1 {
white-space: nowrap;
overflow: hidden;
text-overflow: ellipsis;
max-width: 70%;
color: currentColor;
@media screen and (max-width: #{ ($break-small) }) {
display: none;
}
}

Expand Down
2 changes: 0 additions & 2 deletions packages/editor/src/components/editor-interface/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ export default function EditorInterface( {
customSavePanel,
forceDisableBlockTools,
title,
icon,
iframeProps,
} ) {
const {
Expand Down Expand Up @@ -140,7 +139,6 @@ export default function EditorInterface( {
customSaveButton={ customSaveButton }
forceDisableBlockTools={ forceDisableBlockTools }
title={ title }
icon={ icon }
/>
)
}
Expand Down
3 changes: 1 addition & 2 deletions packages/editor/src/components/header/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,6 @@ function Header( {
forceDisableBlockTools,
setEntitiesSavedStatesCallback,
title,
icon,
} ) {
const zoomOutExperimentEnabled =
window.__experimentalEnableZoomOutExperiment;
Expand Down Expand Up @@ -121,7 +120,7 @@ function Header( {
variants={ toolbarVariations }
transition={ { type: 'tween' } }
>
<DocumentBar title={ title } icon={ icon } />
<DocumentBar title={ title } />
</motion.div>
) }
<motion.div
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/editor/various/post-editor-template-mode.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -196,7 +196,7 @@ class PostEditorTemplateMode {
);

const title = this.editorTopBar.getByRole( 'heading', {
name: 'Editing template: Single Entries',
name: 'Single Entries',
} );

await expect( title ).toBeVisible();
Expand Down
2 changes: 1 addition & 1 deletion test/e2e/specs/site-editor/command-center.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ test.describe( 'Site editor command palette', () => {
page
.getByRole( 'region', { name: 'Editor top bar' } )
.getByRole( 'heading', { level: 1 } )
).toHaveText( 'Index' );
).toContainText( 'Index' );
} );

test( 'Open the command palette and navigate to Customize CSS', async ( {
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/specs/site-editor/template-registration.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ test.describe( 'Block template registration', () => {
} );

// Swap template.
await page.getByRole( 'button', { name: 'Post' } ).click();
await page.getByRole( 'button', { name: 'Post', exact: true } ).click();
await page.getByRole( 'button', { name: 'Template options' } ).click();
await page.getByRole( 'menuitem', { name: 'Swap template' } ).click();
await page.getByText( 'Plugin Template' ).click();
Expand All @@ -135,7 +135,7 @@ test.describe( 'Block template registration', () => {
} );

// Swap template.
await page.getByRole( 'button', { name: 'Post' } ).click();
await page.getByRole( 'button', { name: 'Post', exact: true } ).click();
await page.getByRole( 'button', { name: 'Template options' } ).click();
await page.getByRole( 'menuitem', { name: 'Swap template' } ).click();
await page.getByText( 'Custom', { exact: true } ).click();
Expand Down
4 changes: 2 additions & 2 deletions test/e2e/specs/site-editor/title.spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ test.describe( 'Site editor title', () => {
const title = page
.getByRole( 'region', { name: 'Editor top bar' } )
.getByRole( 'heading', {
name: 'Editing template: Index',
name: 'Index',
} );

await expect( title ).toBeVisible();
Expand All @@ -44,7 +44,7 @@ test.describe( 'Site editor title', () => {
const title = page
.getByRole( 'region', { name: 'Editor top bar' } )
.getByRole( 'heading', {
name: 'Editing template part: header',
name: 'header',
} );

await expect( title ).toBeVisible();
Expand Down
Loading