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

Simplify api for link UI abstraction to use a single prop for the value #46189

Merged
merged 2 commits into from
Nov 30, 2022
Merged

Simplify api for link UI abstraction to use a single prop for the value #46189

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c4077b
Refactor to link prop for all link related data
getdave Nov 30, 2022
99a7294
Apply same refactor to originalk code in Nav Link block
getdave Nov 30, 2022
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
12 changes: 1 addition & 11 deletions packages/block-editor/src/components/off-canvas-editor/appender.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,20 +52,10 @@ export const Appender = forwardRef( ( props, ref ) => {
let maybeLinkUI;

if ( insertedBlock ) {
const link = {
url: insertedBlockAttributes.url,
opensInNewTab: insertedBlockAttributes.opensInNewTab,
title: insertedBlockAttributes.label,
};
Comment on lines -55 to -59
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Previously you needed to know about the structure of this object. Now you just pass all the attributes.

maybeLinkUI = (
<LinkUI
clientId={ insertedBlock }
value={ link }
linkAttributes={ {
type: insertedBlockAttributes.type,
url: insertedBlockAttributes.url,
kind: insertedBlockAttributes.kind,
} }
link={ insertedBlockAttributes }
onClose={ () => setInsertedBlock( null ) }
hasCreateSuggestion={ false }
onChange={ ( updatedValue ) => {
Expand Down
20 changes: 13 additions & 7 deletions packages/block-editor/src/components/off-canvas-editor/link-ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,6 +123,12 @@ function LinkControlTransforms( { clientId } ) {
}

export function LinkUI( props ) {
const link = {
url: props.link.url,
opensInNewTab: props.link.opensInNewTab,
title: props.link.label,
};

return (
<Popover
placement="bottom"
Expand All @@ -131,22 +137,22 @@ export function LinkUI( props ) {
shift
>
<LinkControl
className={ props.className }
hasTextControl
hasRichPreviews
value={ props?.value }
className={ props.className }
value={ link }
showInitialSuggestions={ true }
withCreateSuggestion={ props?.hasCreateSuggestion }
noDirectEntry={ !! props?.linkAttributes?.type }
noURLSuggestion={ !! props?.linkAttributes?.type }
noDirectEntry={ !! props.link?.type }
noURLSuggestion={ !! props.link?.type }
suggestionsQuery={ getSuggestionsQuery(
props?.linkAttributes?.type,
props?.linkAttributes?.kind
props.link?.type,
props.link?.kind
) }
onChange={ props.onChange }
onRemove={ props.onRemove }
renderControlBottom={
! props?.linkAttributes?.url
! props.link?.url
? () => (
<LinkControlTransforms
clientId={ props?.clientId }
Expand Down
37 changes: 2 additions & 35 deletions packages/block-library/src/navigation-link/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,22 +211,6 @@ function getMissingText( type ) {
return missingText;
}

/**
* Removes HTML from a given string.
* Note the does not provide XSS protection or otherwise attempt
* to filter strings with malicious intent.
*
* See also: https://github.com/WordPress/gutenberg/pull/35539
*
* @param {string} html the string from which HTML should be removed.
* @return {string} the "cleaned" string.
*/
function navStripHTML( html ) {
const doc = document.implementation.createHTMLDocument( '' );
doc.body.innerHTML = html;
return doc.body.textContent || '';
}

export default function NavigationLinkEdit( {
attributes,
isSelected,
Expand All @@ -237,27 +221,11 @@ export default function NavigationLinkEdit( {
context,
clientId,
} ) {
const {
id,
label,
type,
opensInNewTab,
url,
description,
rel,
title,
kind,
} = attributes;
const { id, label, type, url, description, rel, title, kind } = attributes;

const [ isInvalid, isDraft ] = useIsInvalidLink( kind, type, id );
const { maxNestingLevel } = context;

const link = {
url,
opensInNewTab,
title: label && navStripHTML( label ), // don't allow HTML to display inside the <LinkControl>
};

const { replaceBlock, __unstableMarkNextChangeAsNotPersistent } =
useDispatch( blockEditorStore );
const [ isLinkOpen, setIsLinkOpen ] = useState( false );
Expand Down Expand Up @@ -656,8 +624,7 @@ export default function NavigationLinkEdit( {
<LinkUI
className="wp-block-navigation-link__inline-link-input"
clientId={ clientId }
value={ link }
linkAttributes={ { type, url, kind } }
link={ attributes }
onClose={ () => setIsLinkOpen( false ) }
anchor={ popoverAnchor }
hasCreateSuggestion={ userCanCreate }
Expand Down
38 changes: 30 additions & 8 deletions packages/block-library/src/navigation-link/link-ui.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -116,11 +116,27 @@ function LinkControlTransforms( { clientId } ) {
);
}

/**
* Removes HTML from a given string.
* Note the does not provide XSS protection or otherwise attempt
* to filter strings with malicious intent.
*
* See also: https://github.com/WordPress/gutenberg/pull/35539
*
* @param {string} html the string from which HTML should be removed.
* @return {string} the "cleaned" string.
*/
function navStripHTML( html ) {
const doc = document.implementation.createHTMLDocument( '' );
doc.body.innerHTML = html;
return doc.body.textContent || '';
}

export function LinkUI( props ) {
const { saveEntityRecord } = useDispatch( coreStore );

async function handleCreate( pageTitle ) {
const postType = props.linkAttributes.type || 'page';
const postType = props.link.type || 'page';

const page = await saveEntityRecord( 'postType', postType, {
title: pageTitle,
Expand All @@ -146,6 +162,12 @@ export function LinkUI( props ) {
};
}

const link = {
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a lot of instances of props.link.whatever here. It might be worth destructuring that variable first so that we can just reference the variables directly. This will mean if we ever want to change the props we pass again we'll have less places to change. Happy to do that in a followup though.

url: props.link.url,
opensInNewTab: props.link.opensInNewTab,
title: props.link.label && navStripHTML( props.link.label ),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@MaggieCabrera and I noticed that the implementation in the offcanvas does not strip the HTML prior to utilising in the Link UI. We need to apply the same logic there.

Also we sholud check whether we can then move both over to using the standard stripHTML from @wordpress/dom or whether there was a particular reason we went with a custom method which is identifcal to stripHTML except from that it doesn't run safeHTML over the string.

Copy link
Contributor Author

@getdave getdave Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I found this comment which suggests moving to the stripHTML would be ok

We'll probably want to hand roll an implementation of this whilst we wait on #35539

Copy link
Contributor

Choose a reason for hiding this comment

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

it also looks like the off-canvas editor is not offering the option to create a new page when it doesn't exist. I don't know if that's intended or not. If not, then that's definitely something to follow up on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it also looks like the off-canvas editor is not offering the option to create a new page when it doesn't exist. I don't know if that's intended or not. If not, then that's definitely something to follow up on.

It's intentional in the sense that I deliberately removed it. Why? Because including that functionality requires saveEntityRecord from @wordpress/core-data and we need to avoid coupling that package to block editor. I don't yet have a good way around this...

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 created #46193 to track.

Copy link
Contributor

Choose a reason for hiding this comment

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

makes sense, thanks!

Copy link
Contributor

@scruffian scruffian Nov 30, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I found this comment which suggests moving to the stripHTML would be ok

Should we create an issue for this too?

};

return (
<Popover
placement="bottom"
Expand All @@ -157,14 +179,14 @@ export function LinkUI( props ) {
hasTextControl
hasRichPreviews
className={ props.className }
value={ props.value }
value={ link }
showInitialSuggestions={ true }
withCreateSuggestion={ props.hasCreateSuggestion }
createSuggestion={ handleCreate }
createSuggestionButtonText={ ( searchTerm ) => {
let format;

if ( props.linkAttributes.type === 'post' ) {
if ( props.link.type === 'post' ) {
/* translators: %s: search term. */
format = __( 'Create draft post: <mark>%s</mark>' );
} else {
Expand All @@ -179,16 +201,16 @@ export function LinkUI( props ) {
}
);
} }
noDirectEntry={ !! props.linkAttributes.type }
noURLSuggestion={ !! props.linkAttributes.type }
noDirectEntry={ !! props.link.type }
noURLSuggestion={ !! props.link.type }
suggestionsQuery={ getSuggestionsQuery(
props.linkAttributes.type,
props.linkAttributes.kind
props.link.type,
props.link.kind
) }
onChange={ props.onChange }
onRemove={ props.onRemove }
renderControlBottom={
! props.linkAttributes.url
! props.link.url
? () => (
<LinkControlTransforms
clientId={ props.clientId }
Expand Down