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

Enhance: Added Dialog Wrapper for EntitiesSavedStatesExtensible #67352

Closed
wants to merge 4 commits into from
Closed

Enhance: Added Dialog Wrapper for EntitiesSavedStatesExtensible #67352

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
fca00ad
feat: added wrapper for dialog behaviour
Mayank-Tripathi32 Nov 27, 2024
8c4caa5
chore: removed renderDialog prop & relating behaviour
Mayank-Tripathi32 Nov 27, 2024
d977929
chore: removed extra props "renderDialog" as no longer required
Mayank-Tripathi32 Nov 27, 2024
0db9b84
chore: updated wrapper name, exported it from entities-saved-states &…
Mayank-Tripathi32 Nov 28, 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
10 changes: 3 additions & 7 deletions packages/edit-site/src/components/save-panel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,13 +80,11 @@ const EntitiesSavedStatesForPreview = ( { onClose } ) => {
);
};

const _EntitiesSavedStates = ( { onClose, renderDialog = undefined } ) => {
const _EntitiesSavedStates = ( { onClose } ) => {
if ( isPreviewingTheme() ) {
return <EntitiesSavedStatesForPreview onClose={ onClose } />;
}
return (
<EntitiesSavedStates close={ onClose } renderDialog={ renderDialog } />
);
return <EntitiesSavedStates close={ onClose } />;
};

export default function SavePanel() {
Expand Down Expand Up @@ -159,9 +157,7 @@ export default function SavePanel() {
{ __( 'Open save panel' ) }
</Button>
</div>
{ isSaveViewOpen && (
<_EntitiesSavedStates onClose={ onClose } renderDialog />
) }
{ isSaveViewOpen && <_EntitiesSavedStates onClose={ onClose } /> }
</NavigableRegion>
);
}
1 change: 0 additions & 1 deletion packages/editor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,6 @@ _Parameters_

- _props_ `Object`: The component props.
- _props.close_ `Function`: The function to close the dialog.
- _props.renderDialog_ `Function`: The function to render the dialog.

_Returns_

Expand Down
47 changes: 7 additions & 40 deletions packages/editor/src/components/entities-saved-states/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,6 @@ import {
useRef,
createInterpolateElement,
} from '@wordpress/element';
import {
__experimentalUseDialog as useDialog,
useInstanceId,
} from '@wordpress/compose';
import { useDispatch } from '@wordpress/data';

/**
Expand All @@ -29,23 +25,15 @@ function identity( values ) {
/**
* Renders the component for managing saved states of entities.
*
* @param {Object} props The component props.
* @param {Function} props.close The function to close the dialog.
* @param {Function} props.renderDialog The function to render the dialog.
* @param {Object} props The component props.
* @param {Function} props.close The function to close the dialog.
*
* @return {JSX.Element} The rendered component.
*/
export default function EntitiesSavedStates( {
close,
renderDialog = undefined,
} ) {
export default function EntitiesSavedStates( { close } ) {
const isDirtyProps = useIsDirty();
return (
<EntitiesSavedStatesExtensible
close={ close }
renderDialog={ renderDialog }
{ ...isDirtyProps }
/>
<EntitiesSavedStatesExtensible close={ close } { ...isDirtyProps } />
);
}

Expand All @@ -58,7 +46,6 @@ export default function EntitiesSavedStates( {
* @param {Function} props.onSave Function to call when saving entities.
* @param {boolean} props.saveEnabled Flag indicating if save is enabled.
* @param {string} props.saveLabel Label for the save button.
* @param {Function} props.renderDialog Function to render a custom dialog.
* @param {Array} props.dirtyEntityRecords Array of dirty entity records.
* @param {boolean} props.isDirty Flag indicating if there are dirty entities.
* @param {Function} props.setUnselectedEntities Function to set unselected entities.
Expand All @@ -72,7 +59,6 @@ export function EntitiesSavedStatesExtensible( {
onSave = identity,
saveEnabled: saveEnabledProp = undefined,
saveLabel = __( 'Save' ),
renderDialog = undefined,
dirtyEntityRecords,
isDirty,
setUnselectedEntities,
Expand Down Expand Up @@ -109,24 +95,8 @@ export function EntitiesSavedStatesExtensible( {
// its own will use the event object in place of the expected saved entities.
const dismissPanel = useCallback( () => close(), [ close ] );

const [ saveDialogRef, saveDialogProps ] = useDialog( {
onClose: () => dismissPanel(),
} );
const dialogLabel = useInstanceId( EntitiesSavedStatesExtensible, 'label' );
const dialogDescription = useInstanceId(
EntitiesSavedStatesExtensible,
'description'
);

return (
<div
ref={ saveDialogRef }
{ ...saveDialogProps }
className="entities-saved-states__panel"
role={ renderDialog ? 'dialog' : undefined }
aria-labelledby={ renderDialog ? dialogLabel : undefined }
aria-describedby={ renderDialog ? dialogDescription : undefined }
>
<div className="entities-saved-states__panel">
<Flex className="entities-saved-states__panel-header" gap={ 2 }>
<FlexItem
isBlock
Expand Down Expand Up @@ -160,16 +130,13 @@ export function EntitiesSavedStatesExtensible( {
</Flex>

<div className="entities-saved-states__text-prompt">
<div
className="entities-saved-states__text-prompt--header-wrapper"
id={ renderDialog ? dialogLabel : undefined }
Copy link
Contributor

Choose a reason for hiding this comment

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

removing this id will make the dialog labeling fail

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking that might be the case, Might have to look into an alternate way to pass down Ids maybe?

Copy link
Contributor

Choose a reason for hiding this comment

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

yes but I would argue that given we need to pass down the unique ID, how this would be better than using the current approach that passed down the renderDialog prop? The only difference would be that the logic is abstracted in a wrapper component but I'm not sure that would be a substantial improvement code-wise.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True, but looking at the original code, it seems that the aria tags were missing anyway. I’m not entirely sure how much impact the aria tags would have, but we could consider #67351

Copy link
Contributor

Choose a reason for hiding this comment

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

True, they were missing because the renderDialog prop was not passe down correctly. Looking at the original PR that introduced thie behavior #59622, the intent is to make all 'Save panels' behave like a sort of modal dialog and use the useDialog props and also the IDs for aria-labelledby and aria-describedby.

>
<div className="entities-saved-states__text-prompt--header-wrapper">
<strong className="entities-saved-states__text-prompt--header">
{ __( 'Are you ready to save?' ) }
</strong>
{ additionalPrompt }
</div>
<p id={ renderDialog ? dialogDescription : undefined }>
Copy link
Contributor

Choose a reason for hiding this comment

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

removing this id will make the dialog aria-describedby fail

<p>
{ isDirty
? createInterpolateElement(
sprintf(
Expand Down
47 changes: 45 additions & 2 deletions packages/editor/src/components/save-publish-panels/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,17 @@ import { useSelect, useDispatch } from '@wordpress/data';
import { Button, createSlotFill } from '@wordpress/components';
import { __ } from '@wordpress/i18n';
import { useCallback } from '@wordpress/element';
import {
__experimentalUseDialog as useDialog,
useInstanceId,
} from '@wordpress/compose';

/**
* Internal dependencies
*/
import EntitiesSavedStates from '../entities-saved-states';
import EntitiesSavedStates, {
EntitiesSavedStatesExtensible,
} from '../entities-saved-states';
import PostPublishPanel from '../post-publish-panel';
import PluginPrePublishPanel from '../plugin-pre-publish-panel';
import PluginPostPublishPanel from '../plugin-post-publish-panel';
Expand Down Expand Up @@ -102,10 +108,47 @@ export default function SavePublishPanels( {
return (
<>
{ isEntitiesSavedStatesOpen && (
<EntitiesSavedStates close={ closeEntitiesSavedStates } />
<EntitieDialogWrapper close={ closeEntitiesSavedStates }>
<EntitiesSavedStates close={ closeEntitiesSavedStates } />
</EntitieDialogWrapper>
) }
<Slot bubblesVirtually />
{ ! isEntitiesSavedStatesOpen && unmountableContent }
</>
);
}

/**
* A wrapper component that renders a dialog for displaying entities.
*
* @param {Object} props The component's props.
* @param {React.ReactNode} props.children The content to be displayed inside the dialog.
* @param {Function} props.close A function to close the dialog.
*
* @return {React.Element} The rendered dialog element with children.
*/
export function EntitieDialogWrapper( { children, close } ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

this new wrapper should be used in other palces as well, for example for the save panel in the theme preview, see EntitiesSavedStatesForPreview

Copy link
Contributor

Choose a reason for hiding this comment

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

The name of the component should make clear it is meant as a wrapper for EntitiesSavedStates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. I only worked on save panel as If it was acceptable solution then probably can add to theme preview aswell.

const dismissPanel = useCallback( () => close(), [ close ] );
const [ saveDialogRef, saveDialogProps ] = useDialog( {
onClose: () => dismissPanel(),
} );

const dialogLabel = useInstanceId( EntitiesSavedStatesExtensible, 'label' );
const dialogDescription = useInstanceId(
EntitiesSavedStatesExtensible,
'description'
);

return (
<div
ref={ saveDialogRef }
{ ...saveDialogProps }
role="dialog"
aria-labelledby={ dialogLabel }
aria-describedby={ dialogDescription }
>
{ ' ' }
Copy link
Contributor

Choose a reason for hiding this comment

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

Why this empty space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

seems to be added by npm run format, will remove it now. Thanks

{ children }
</div>
);
}
Loading