From 6271874b05bd42eefb47223fc4056091f5cefada Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 20 Sep 2023 13:36:41 +0100 Subject: [PATCH 01/12] Initial mapping functions and tests --- .../link-control/link-value-transforms.js | 28 ++++++ .../test/link-value-transforms.js | 97 +++++++++++++++++++ 2 files changed, 125 insertions(+) create mode 100644 packages/block-editor/src/components/link-control/link-value-transforms.js create mode 100644 packages/block-editor/src/components/link-control/test/link-value-transforms.js diff --git a/packages/block-editor/src/components/link-control/link-value-transforms.js b/packages/block-editor/src/components/link-control/link-value-transforms.js new file mode 100644 index 0000000000000..6d555abf4ff51 --- /dev/null +++ b/packages/block-editor/src/components/link-control/link-value-transforms.js @@ -0,0 +1,28 @@ +export function buildLinkValueFromData( data, mapping ) { + const linkValue = {}; + for ( const [ attributeName, valueGetter ] of Object.entries( mapping ) ) { + if ( typeof valueGetter === 'string' ) { + linkValue[ attributeName ] = data[ valueGetter ]; + } else { + linkValue[ attributeName ] = valueGetter.toLink( + data[ valueGetter.dataKey ] + ); + } + } + return linkValue; +} + +export function buildDataFromLinkValue( linkValue, mapping ) { + const data = {}; + for ( const [ attributeName, valueGetter ] of Object.entries( mapping ) ) { + if ( typeof valueGetter === 'string' ) { + data[ valueGetter ] = linkValue[ attributeName ]; + } else { + data[ valueGetter.dataKey ] = valueGetter.toData( + linkValue[ attributeName ], + data[ valueGetter.dataKey ] + ); + } + } + return data; +} diff --git a/packages/block-editor/src/components/link-control/test/link-value-transforms.js b/packages/block-editor/src/components/link-control/test/link-value-transforms.js new file mode 100644 index 0000000000000..45d8e6c609c2a --- /dev/null +++ b/packages/block-editor/src/components/link-control/test/link-value-transforms.js @@ -0,0 +1,97 @@ +/** + * Internal dependencies + */ +import { + buildLinkValueFromData, + buildDataFromLinkValue, +} from '../link-value-transforms'; + +/** + * Maps the standard LinkControl values to a given data object. + * Complex mappings may supply an object with a `getter` and `setter` function + * which represent how to get the link value for the given property and how to + * set it back on the data object. + */ +const mapping = { + url: 'href', + type: 'postType', + id: 'id', + opensInNewTab: { + dataKey: 'linkTarget', + toLink: ( value ) => value === '_blank', + toData: ( value ) => ( value ? '_blank' : undefined ), + }, + noFollow: { + dataKey: 'linkRel', + toLink: ( value ) => value.includes( 'nofollow' ), + toData: ( value, currentVal ) => { + // if the value is truthy and the current value is set + // then append otherwise just add the value + if ( value && currentVal ) { + return `${ currentVal } nofollow`; + } else if ( value ) { + return 'nofollow'; + } + }, + }, + sponsored: { + dataKey: 'linkRel', + toLink: ( value ) => value.includes( 'sponsored' ), + toData: ( value, currentVal ) => { + // if the value is truthy and the current value is set + // then append otherwise just add the value + if ( value && currentVal ) { + return `${ currentVal } sponsored`; + } else if ( value ) { + return 'sponsored'; + } + }, + }, +}; + +describe( 'buildLinkValueFromData', () => { + it( 'build a valid link value from supplied data mapping', () => { + const data = { + href: 'https://www.google.com', + postType: 'post', + id: 123, + linkTarget: '_blank', + linkRel: 'nofollow noopenner sponsored', + keyToIgnore: 'valueToIgnore', + }; + + const linkValue = buildLinkValueFromData( data, mapping ); + + expect( linkValue ).toEqual( { + url: 'https://www.google.com', + type: 'post', + id: 123, + opensInNewTab: true, + noFollow: true, + sponsored: true, + } ); + } ); +} ); + +describe( 'buildDataFromLinkValue', () => { + it( 'build a valid data object from supplied link value mapping', () => { + const linkValue = { + url: 'https://www.google.com', + type: 'post', + id: 123, + opensInNewTab: true, + noFollow: true, + sponsored: true, + }; + + const data = buildDataFromLinkValue( linkValue, mapping ); + + expect( data ).toEqual( { + href: 'https://www.google.com', + postType: 'post', + id: 123, + linkTarget: '_blank', + linkRel: 'nofollow sponsored', + } ); + } ); +} ); From 5a9914d298a3499abe6503954c69552ff1ba08a5 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 20 Sep 2023 13:40:35 +0100 Subject: [PATCH 02/12] Lay foundation for more complex tests --- .../test/link-value-transforms.js | 62 +++++++++++++------ 1 file changed, 42 insertions(+), 20 deletions(-) diff --git a/packages/block-editor/src/components/link-control/test/link-value-transforms.js b/packages/block-editor/src/components/link-control/test/link-value-transforms.js index 45d8e6c609c2a..5713554be9ab9 100644 --- a/packages/block-editor/src/components/link-control/test/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/test/link-value-transforms.js @@ -50,27 +50,49 @@ const mapping = { }; describe( 'buildLinkValueFromData', () => { - it( 'build a valid link value from supplied data mapping', () => { - const data = { - href: 'https://www.google.com', - postType: 'post', - id: 123, - linkTarget: '_blank', - linkRel: 'nofollow noopenner sponsored', - keyToIgnore: 'valueToIgnore', - }; - - const linkValue = buildLinkValueFromData( data, mapping ); + it.each( [ + [ + { + href: 'https://www.google.com', + postType: 'post', + id: 123, + linkTarget: '_blank', + linkRel: 'nofollow noopenner sponsored', + keyToIgnore: 'valueToIgnore', + }, + { + url: 'https://www.google.com', + type: 'post', + id: 123, + opensInNewTab: true, + noFollow: true, + sponsored: true, + }, + ], + [ + { + href: 'https://www.google.com', + postType: 'post', + id: 123, + linkRel: 'sponsored neyfollow', + }, + { + url: 'https://www.google.com', + type: 'post', + id: 123, + opensInNewTab: false, + noFollow: false, + sponsored: true, + }, + ], + ] )( + 'build a valid link value from supplied data mapping', + ( data, expected ) => { + const linkValue = buildLinkValueFromData( data, mapping ); - expect( linkValue ).toEqual( { - url: 'https://www.google.com', - type: 'post', - id: 123, - opensInNewTab: true, - noFollow: true, - sponsored: true, - } ); - } ); + expect( linkValue ).toEqual( expected ); + } + ); } ); describe( 'buildDataFromLinkValue', () => { From aa811f8161545e92811d047206daad9cf2746777 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Wed, 20 Sep 2023 14:00:03 +0100 Subject: [PATCH 03/12] Export data mapping utils --- packages/block-editor/README.md | 8 ++++++++ packages/block-editor/src/components/index.js | 4 ++++ 2 files changed, 12 insertions(+) diff --git a/packages/block-editor/README.md b/packages/block-editor/README.md index 063a96384708c..b8104ddb5ae82 100644 --- a/packages/block-editor/README.md +++ b/packages/block-editor/README.md @@ -306,6 +306,14 @@ _Related_ - +### buildDataFromLinkValue + +Undocumented declaration. + +### buildLinkValueFromData + +Undocumented declaration. + ### ButtonBlockAppender _Related_ diff --git a/packages/block-editor/src/components/index.js b/packages/block-editor/src/components/index.js index 1622cecc150ed..c18b068e05341 100644 --- a/packages/block-editor/src/components/index.js +++ b/packages/block-editor/src/components/index.js @@ -71,6 +71,10 @@ export { default as __experimentalLinkControl } from './link-control'; export { default as __experimentalLinkControlSearchInput } from './link-control/search-input'; export { default as __experimentalLinkControlSearchResults } from './link-control/search-results'; export { default as __experimentalLinkControlSearchItem } from './link-control/search-item'; +export { + buildLinkValueFromData, + buildDataFromLinkValue, +} from './link-control/link-value-transforms'; export { default as LineHeightControl } from './line-height-control'; export { default as __experimentalListView } from './list-view'; export { default as MediaReplaceFlow } from './media-replace-flow'; From 13071ae02e3a090b701d2dc5e5a726c7343f6413 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 29 Sep 2023 11:59:11 +0100 Subject: [PATCH 04/12] Pass full link object as secon param for full utility --- .../src/components/link-control/link-value-transforms.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/components/link-control/link-value-transforms.js b/packages/block-editor/src/components/link-control/link-value-transforms.js index 6d555abf4ff51..7ecb78b209aac 100644 --- a/packages/block-editor/src/components/link-control/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/link-value-transforms.js @@ -5,7 +5,8 @@ export function buildLinkValueFromData( data, mapping ) { linkValue[ attributeName ] = data[ valueGetter ]; } else { linkValue[ attributeName ] = valueGetter.toLink( - data[ valueGetter.dataKey ] + data[ valueGetter.dataKey ], + data ); } } @@ -20,7 +21,7 @@ export function buildDataFromLinkValue( linkValue, mapping ) { } else { data[ valueGetter.dataKey ] = valueGetter.toData( linkValue[ attributeName ], - data[ valueGetter.dataKey ] + linkValue ); } } From e8526cd9640e6640a169ed96ed070d9867ef8292 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 29 Sep 2023 12:00:08 +0100 Subject: [PATCH 05/12] Re-implement link updating using standardised utils --- packages/block-library/src/button/edit.js | 89 ++++++++++++++++++----- 1 file changed, 70 insertions(+), 19 deletions(-) diff --git a/packages/block-library/src/button/edit.js b/packages/block-library/src/button/edit.js index 81861e44997a4..363b71e4a7d42 100644 --- a/packages/block-library/src/button/edit.js +++ b/packages/block-library/src/button/edit.js @@ -6,14 +6,13 @@ import classnames from 'classnames'; /** * Internal dependencies */ -import { NEW_TAB_TARGET, NOFOLLOW_REL } from './constants'; -import { getUpdatedLinkAttributes } from './get-updated-link-attributes'; +import { NEW_TAB_TARGET, NOFOLLOW_REL, NEW_TAB_REL } from './constants'; /** * WordPress dependencies */ import { __ } from '@wordpress/i18n'; -import { useEffect, useState, useRef, useMemo } from '@wordpress/element'; +import { useEffect, useState, useRef } from '@wordpress/element'; import { Button, ButtonGroup, @@ -32,6 +31,8 @@ import { __experimentalUseColorProps as useColorProps, __experimentalGetSpacingClassesAndStyles as useSpacingProps, __experimentalLinkControl as LinkControl, + buildLinkValueFromData, + buildDataFromLinkValue, __experimentalGetElementClassName, } from '@wordpress/block-editor'; import { displayShortcut, isKeyboardEvent } from '@wordpress/keycodes'; @@ -133,10 +134,60 @@ function ButtonEdit( props ) { const [ isEditingURL, setIsEditingURL ] = useState( false ); const isURLSet = !! url; - const opensInNewTab = linkTarget === NEW_TAB_TARGET; - const nofollow = !! rel?.includes( NOFOLLOW_REL ); + const isLinkTag = 'a' === TagName; + // Defines how block attributes map to link value and vice versa. + const linkValueAttrsToDataMapping = { + url: 'url', + opensInNewTab: { + dataKey: 'linkTarget', + toLink: ( value ) => value === NEW_TAB_TARGET, + toData: ( value ) => ( value ? NEW_TAB_TARGET : undefined ), + }, + nofollow: { + dataKey: 'rel', + toLink: ( value ) => value?.includes( NOFOLLOW_REL ), + toData: ( value, { opensInNewTab: opensInNewTabValue } ) => { + // "rel" attribute can be effected by changes to + // "nofollow" and "opensInNewTab" attributes. + // In addition it is editable in plaintext via the UI + // so consider that it may already contain a value. + let updatedRel = ''; + + // Handle setting rel based on nofollow setting. + if ( value ) { + updatedRel = updatedRel?.includes( NOFOLLOW_REL ) + ? updatedRel + : updatedRel + ` ${ NOFOLLOW_REL }`; + } else { + const relRegex = new RegExp( + `\\b${ NOFOLLOW_REL }\\s*`, + 'g' + ); + updatedRel = updatedRel?.replace( relRegex, '' ).trim(); + } + + // Handle setting rel based on opensInNewTab setting. + if ( opensInNewTabValue ) { + updatedRel = updatedRel?.includes( NEW_TAB_REL ) + ? updatedRel + : updatedRel + ` ${ NEW_TAB_REL }`; + } else { + const relRegex = new RegExp( + `\\b${ NEW_TAB_REL }\\s*`, + 'g' + ); + updatedRel = updatedRel?.replace( relRegex, '' ).trim(); + } + + // Returning `undefined` here if there is no String-based rel value. + // ensures that the attribute is fully removed from the block. + return updatedRel || undefined; + }, + }, + }; + function startEditing( event ) { event.preventDefault(); setIsEditingURL( true ); @@ -157,12 +208,18 @@ function ButtonEdit( props ) { } }, [ isSelected ] ); + // NOT NOT MERGE WITHOUT RE-INTSTAINTG THIS MEMOIZATION // Memoize link value to avoid overriding the LinkControl's internal state. // This is a temporary fix. See https://github.com/WordPress/gutenberg/issues/51256. - const linkValue = useMemo( - () => ( { url, opensInNewTab, nofollow } ), - [ url, opensInNewTab, nofollow ] + const linkValue = buildLinkValueFromData( + { + url, + linkTarget, + rel, + }, + linkValueAttrsToDataMapping ); + // NOT NOT MERGE WITHOUT RE-INTSTAINTG THIS MEMOIZATION return ( <> @@ -251,18 +308,12 @@ function ButtonEdit( props ) { > + onChange={ ( newLinkValue ) => setAttributes( - getUpdatedLinkAttributes( { - rel, - url: newURL, - opensInNewTab: newOpensInNewTab, - nofollow: newNofollow, - } ) + buildDataFromLinkValue( + newLinkValue, + linkValueAttrsToDataMapping + ) ) } onRemove={ () => { From 1edac52fc838caad10ebbccfd782bf5f52b988af Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 29 Sep 2023 13:34:02 +0100 Subject: [PATCH 06/12] Ensure http is prepended to any stored URL --- packages/block-library/src/button/edit.js | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/packages/block-library/src/button/edit.js b/packages/block-library/src/button/edit.js index 363b71e4a7d42..2f62b40a423b2 100644 --- a/packages/block-library/src/button/edit.js +++ b/packages/block-library/src/button/edit.js @@ -39,6 +39,7 @@ import { displayShortcut, isKeyboardEvent } from '@wordpress/keycodes'; import { link, linkOff } from '@wordpress/icons'; import { createBlock } from '@wordpress/blocks'; import { useMergeRefs } from '@wordpress/compose'; +import { prependHTTP } from '@wordpress/url'; const LINK_SETTINGS = [ ...LinkControl.DEFAULT_LINK_SETTINGS, @@ -139,7 +140,10 @@ function ButtonEdit( props ) { // Defines how block attributes map to link value and vice versa. const linkValueAttrsToDataMapping = { - url: 'url', + url: { + dataKey: 'url', + toData: ( value ) => prependHTTP( value ), + }, opensInNewTab: { dataKey: 'linkTarget', toLink: ( value ) => value === NEW_TAB_TARGET, From b98a8a90847591644682c506841e3d49b6377d91 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 6 Oct 2023 10:00:58 +0100 Subject: [PATCH 07/12] Export single build function --- packages/block-editor/README.md | 12 ++++-------- packages/block-editor/src/components/index.js | 5 +---- .../link-control/link-value-transforms.js | 11 +++++++++-- .../link-control/test/link-value-transforms.js | 14 ++++++++------ 4 files changed, 22 insertions(+), 20 deletions(-) diff --git a/packages/block-editor/README.md b/packages/block-editor/README.md index b8104ddb5ae82..1c7c053a76d26 100644 --- a/packages/block-editor/README.md +++ b/packages/block-editor/README.md @@ -306,14 +306,6 @@ _Related_ - -### buildDataFromLinkValue - -Undocumented declaration. - -### buildLinkValueFromData - -Undocumented declaration. - ### ButtonBlockAppender _Related_ @@ -540,6 +532,10 @@ _Returns_ - `string`: Gradient value. +### getLinkValueTransforms + +Undocumented declaration. + ### getPxFromCssUnit Returns the px value of a cssUnit. The memoized version of getPxFromCssUnit; diff --git a/packages/block-editor/src/components/index.js b/packages/block-editor/src/components/index.js index c18b068e05341..a354a3e455101 100644 --- a/packages/block-editor/src/components/index.js +++ b/packages/block-editor/src/components/index.js @@ -71,10 +71,7 @@ export { default as __experimentalLinkControl } from './link-control'; export { default as __experimentalLinkControlSearchInput } from './link-control/search-input'; export { default as __experimentalLinkControlSearchResults } from './link-control/search-results'; export { default as __experimentalLinkControlSearchItem } from './link-control/search-item'; -export { - buildLinkValueFromData, - buildDataFromLinkValue, -} from './link-control/link-value-transforms'; +export { default as getLinkValueTransforms } from './link-control/link-value-transforms'; export { default as LineHeightControl } from './line-height-control'; export { default as __experimentalListView } from './list-view'; export { default as MediaReplaceFlow } from './media-replace-flow'; diff --git a/packages/block-editor/src/components/link-control/link-value-transforms.js b/packages/block-editor/src/components/link-control/link-value-transforms.js index 7ecb78b209aac..24676ffe7a09a 100644 --- a/packages/block-editor/src/components/link-control/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/link-value-transforms.js @@ -1,4 +1,4 @@ -export function buildLinkValueFromData( data, mapping ) { +function buildLinkValueFromData( data, mapping ) { const linkValue = {}; for ( const [ attributeName, valueGetter ] of Object.entries( mapping ) ) { if ( typeof valueGetter === 'string' ) { @@ -13,7 +13,7 @@ export function buildLinkValueFromData( data, mapping ) { return linkValue; } -export function buildDataFromLinkValue( linkValue, mapping ) { +function buildDataFromLinkValue( linkValue, mapping ) { const data = {}; for ( const [ attributeName, valueGetter ] of Object.entries( mapping ) ) { if ( typeof valueGetter === 'string' ) { @@ -27,3 +27,10 @@ export function buildDataFromLinkValue( linkValue, mapping ) { } return data; } + +export default function getLinkValueTransforms( mapping ) { + return { + toLink: ( data ) => buildLinkValueFromData( data, mapping ), + toData: ( linkValue ) => buildDataFromLinkValue( linkValue, mapping ), + }; +} diff --git a/packages/block-editor/src/components/link-control/test/link-value-transforms.js b/packages/block-editor/src/components/link-control/test/link-value-transforms.js index 5713554be9ab9..f53dc5e2226bb 100644 --- a/packages/block-editor/src/components/link-control/test/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/test/link-value-transforms.js @@ -1,10 +1,7 @@ /** * Internal dependencies */ -import { - buildLinkValueFromData, - buildDataFromLinkValue, -} from '../link-value-transforms'; +import getLinkValueTransforms from '../link-value-transforms'; /** * Maps the standard LinkControl values to a given data object. @@ -88,7 +85,9 @@ describe( 'buildLinkValueFromData', () => { ] )( 'build a valid link value from supplied data mapping', ( data, expected ) => { - const linkValue = buildLinkValueFromData( data, mapping ); + const { toLink } = getLinkValueTransforms( mapping ); + + const linkValue = toLink( data ); expect( linkValue ).toEqual( expected ); } @@ -106,7 +105,10 @@ describe( 'buildDataFromLinkValue', () => { sponsored: true, }; - const data = buildDataFromLinkValue( linkValue, mapping ); + // const data = buildDataFromLinkValue( linkValue, mapping ); + + const { toData } = getLinkValueTransforms( mapping ); + const data = toData( linkValue ); expect( data ).toEqual( { href: 'https://www.google.com', From 49cf4faed005c399baad55a55fb4578d60b02b91 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 6 Oct 2023 10:32:46 +0100 Subject: [PATCH 08/12] Fix tests by passing full data value to reducer --- .../components/link-control/link-value-transforms.js | 3 ++- .../link-control/test/link-value-transforms.js | 12 ++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/packages/block-editor/src/components/link-control/link-value-transforms.js b/packages/block-editor/src/components/link-control/link-value-transforms.js index 24676ffe7a09a..4394f761da063 100644 --- a/packages/block-editor/src/components/link-control/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/link-value-transforms.js @@ -21,7 +21,8 @@ function buildDataFromLinkValue( linkValue, mapping ) { } else { data[ valueGetter.dataKey ] = valueGetter.toData( linkValue[ attributeName ], - linkValue + linkValue, + data ); } } diff --git a/packages/block-editor/src/components/link-control/test/link-value-transforms.js b/packages/block-editor/src/components/link-control/test/link-value-transforms.js index f53dc5e2226bb..ec4061e3c3655 100644 --- a/packages/block-editor/src/components/link-control/test/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/test/link-value-transforms.js @@ -21,11 +21,11 @@ const mapping = { noFollow: { dataKey: 'linkRel', toLink: ( value ) => value.includes( 'nofollow' ), - toData: ( value, currentVal ) => { + toData: ( value, _, { linkRel: currentLinkRel } ) => { // if the value is truthy and the current value is set // then append otherwise just add the value - if ( value && currentVal ) { - return `${ currentVal } nofollow`; + if ( value && currentLinkRel ) { + return `${ currentLinkRel } nofollow`; } else if ( value ) { return 'nofollow'; } @@ -34,11 +34,11 @@ const mapping = { sponsored: { dataKey: 'linkRel', toLink: ( value ) => value.includes( 'sponsored' ), - toData: ( value, currentVal ) => { + toData: ( value, _, { linkRel: currentLinkRel } ) => { // if the value is truthy and the current value is set // then append otherwise just add the value - if ( value && currentVal ) { - return `${ currentVal } sponsored`; + if ( value && currentLinkRel ) { + return `${ currentLinkRel } sponsored`; } else if ( value ) { return 'sponsored'; } From 855c646c6a8e3d0415ec42073d458dee98dc7623 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 6 Oct 2023 10:32:58 +0100 Subject: [PATCH 09/12] Update Button implementation to match API --- packages/block-library/src/button/edit.js | 27 +++++++++-------------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/packages/block-library/src/button/edit.js b/packages/block-library/src/button/edit.js index 2f62b40a423b2..4de072efc903a 100644 --- a/packages/block-library/src/button/edit.js +++ b/packages/block-library/src/button/edit.js @@ -31,8 +31,7 @@ import { __experimentalUseColorProps as useColorProps, __experimentalGetSpacingClassesAndStyles as useSpacingProps, __experimentalLinkControl as LinkControl, - buildLinkValueFromData, - buildDataFromLinkValue, + getLinkValueTransforms, __experimentalGetElementClassName, } from '@wordpress/block-editor'; import { displayShortcut, isKeyboardEvent } from '@wordpress/keycodes'; @@ -192,6 +191,10 @@ function ButtonEdit( props ) { }, }; + const { toLink, toData } = getLinkValueTransforms( + linkValueAttrsToDataMapping + ); + function startEditing( event ) { event.preventDefault(); setIsEditingURL( true ); @@ -215,14 +218,11 @@ function ButtonEdit( props ) { // NOT NOT MERGE WITHOUT RE-INTSTAINTG THIS MEMOIZATION // Memoize link value to avoid overriding the LinkControl's internal state. // This is a temporary fix. See https://github.com/WordPress/gutenberg/issues/51256. - const linkValue = buildLinkValueFromData( - { - url, - linkTarget, - rel, - }, - linkValueAttrsToDataMapping - ); + const linkValue = toLink( { + url, + linkTarget, + rel, + } ); // NOT NOT MERGE WITHOUT RE-INTSTAINTG THIS MEMOIZATION return ( @@ -313,12 +313,7 @@ function ButtonEdit( props ) { - setAttributes( - buildDataFromLinkValue( - newLinkValue, - linkValueAttrsToDataMapping - ) - ) + setAttributes( toData( newLinkValue ) ) } onRemove={ () => { unlink(); From 13be3c6c0332c4b8a86f309e4081af4f82de6f25 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 6 Oct 2023 11:10:56 +0100 Subject: [PATCH 10/12] Add ability to selectively omit transforms --- .../link-control/link-value-transforms.js | 12 +++- .../test/link-value-transforms.js | 56 ++++++++++++++++--- 2 files changed, 58 insertions(+), 10 deletions(-) diff --git a/packages/block-editor/src/components/link-control/link-value-transforms.js b/packages/block-editor/src/components/link-control/link-value-transforms.js index 4394f761da063..09df4441258f0 100644 --- a/packages/block-editor/src/components/link-control/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/link-value-transforms.js @@ -1,13 +1,19 @@ +function isCallable( value ) { + return value && typeof value === 'function'; +} + function buildLinkValueFromData( data, mapping ) { const linkValue = {}; for ( const [ attributeName, valueGetter ] of Object.entries( mapping ) ) { if ( typeof valueGetter === 'string' ) { linkValue[ attributeName ] = data[ valueGetter ]; - } else { + } else if ( isCallable( valueGetter.toLink ) ) { linkValue[ attributeName ] = valueGetter.toLink( data[ valueGetter.dataKey ], data ); + } else { + linkValue[ attributeName ] = data[ valueGetter.dataKey ]; } } return linkValue; @@ -18,12 +24,14 @@ function buildDataFromLinkValue( linkValue, mapping ) { for ( const [ attributeName, valueGetter ] of Object.entries( mapping ) ) { if ( typeof valueGetter === 'string' ) { data[ valueGetter ] = linkValue[ attributeName ]; - } else { + } else if ( isCallable( valueGetter.toData ) ) { data[ valueGetter.dataKey ] = valueGetter.toData( linkValue[ attributeName ], linkValue, data ); + } else { + data[ valueGetter.dataKey ] = linkValue[ attributeName ]; } } return data; diff --git a/packages/block-editor/src/components/link-control/test/link-value-transforms.js b/packages/block-editor/src/components/link-control/test/link-value-transforms.js index ec4061e3c3655..4a073e0e02585 100644 --- a/packages/block-editor/src/components/link-control/test/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/test/link-value-transforms.js @@ -3,6 +3,10 @@ */ import getLinkValueTransforms from '../link-value-transforms'; +function identity( value ) { + return value; +} + /** * Maps the standard LinkControl values to a given data object. * Complex mappings may supply an object with a `getter` and `setter` function @@ -50,7 +54,7 @@ describe( 'buildLinkValueFromData', () => { it.each( [ [ { - href: 'https://www.google.com', + href: 'https://www.wordpress.org', postType: 'post', id: 123, linkTarget: '_blank', @@ -58,7 +62,7 @@ describe( 'buildLinkValueFromData', () => { keyToIgnore: 'valueToIgnore', }, { - url: 'https://www.google.com', + url: 'https://www.wordpress.org', type: 'post', id: 123, opensInNewTab: true, @@ -68,13 +72,13 @@ describe( 'buildLinkValueFromData', () => { ], [ { - href: 'https://www.google.com', + href: 'https://www.wordpress.org', postType: 'post', id: 123, linkRel: 'sponsored neyfollow', }, { - url: 'https://www.google.com', + url: 'https://www.wordpress.org', type: 'post', id: 123, opensInNewTab: false, @@ -92,12 +96,31 @@ describe( 'buildLinkValueFromData', () => { expect( linkValue ).toEqual( expected ); } ); + + it( 'returns raw data attribute value when toLink transform is not callable', () => { + const { toLink } = getLinkValueTransforms( { + url: { + dataKey: 'href', + // allows toLink to be ommitted in case of simple mapping + // but still allows toData to be defined. + toData: identity, + }, + } ); + + const linkValue = toLink( { + href: 'https://www.wordpress.org', + } ); + + expect( linkValue ).toEqual( { + url: 'https://www.wordpress.org', + } ); + } ); } ); describe( 'buildDataFromLinkValue', () => { it( 'build a valid data object from supplied link value mapping', () => { const linkValue = { - url: 'https://www.google.com', + url: 'https://www.wordpress.org', type: 'post', id: 123, opensInNewTab: true, @@ -105,17 +128,34 @@ describe( 'buildDataFromLinkValue', () => { sponsored: true, }; - // const data = buildDataFromLinkValue( linkValue, mapping ); - const { toData } = getLinkValueTransforms( mapping ); const data = toData( linkValue ); expect( data ).toEqual( { - href: 'https://www.google.com', + href: 'https://www.wordpress.org', postType: 'post', id: 123, linkTarget: '_blank', linkRel: 'nofollow sponsored', } ); } ); + + it( 'returns raw link value attribute when toData transform is not callable', () => { + const { toData } = getLinkValueTransforms( { + url: { + dataKey: 'href', + // allows toData to be ommitted in case of simple mapping + // but still allows toLink to be defined. + toLink: identity, // added for example purposes. + }, + } ); + + const data = toData( { + url: 'https://www.wordpress.org', + } ); + + expect( data ).toEqual( { + href: 'https://www.wordpress.org', + } ); + } ); } ); From 63a5b19752df2c5d4b4bbefe4015ab583518b287 Mon Sep 17 00:00:00 2001 From: Dave Smith Date: Fri, 6 Oct 2023 11:11:54 +0100 Subject: [PATCH 11/12] WIP start updating Nav Link block transforms --- .../block-library/src/navigation-link/edit.js | 6 +- .../src/navigation-link/link-ui.js | 89 +++++++++++++++++-- 2 files changed, 81 insertions(+), 14 deletions(-) diff --git a/packages/block-library/src/navigation-link/edit.js b/packages/block-library/src/navigation-link/edit.js index 4a902871abdee..1298d7d94999c 100644 --- a/packages/block-library/src/navigation-link/edit.js +++ b/packages/block-library/src/navigation-link/edit.js @@ -575,11 +575,7 @@ export default function NavigationLinkEdit( { anchor={ popoverAnchor } onRemove={ removeLink } onChange={ ( updatedValue ) => { - updateAttributes( - updatedValue, - setAttributes, - attributes - ); + setAttributes( updatedValue ); } } /> ) } diff --git a/packages/block-library/src/navigation-link/link-ui.js b/packages/block-library/src/navigation-link/link-ui.js index 3c3d91e7b0a05..8fcf28b7a9766 100644 --- a/packages/block-library/src/navigation-link/link-ui.js +++ b/packages/block-library/src/navigation-link/link-ui.js @@ -6,6 +6,7 @@ import { Popover, Button } from '@wordpress/components'; import { __, sprintf } from '@wordpress/i18n'; import { __experimentalLinkControl as LinkControl, + getLinkValueTransforms, BlockIcon, store as blockEditorStore, } from '@wordpress/block-editor'; @@ -17,6 +18,8 @@ import { import { decodeEntities } from '@wordpress/html-entities'; import { switchToBlockType } from '@wordpress/blocks'; import { useSelect, useDispatch } from '@wordpress/data'; +import { escapeHTML } from '@wordpress/escape-html'; +import { safeDecodeURI } from '@wordpress/url'; /** * Given the Link block's type attribute, return the query params to give to @@ -177,14 +180,82 @@ export function LinkUI( props ) { // Memoize link value to avoid overriding the LinkControl's internal state. // This is a temporary fix. See https://github.com/WordPress/gutenberg/issues/50976#issuecomment-1568226407. - const link = useMemo( - () => ( { - url, - opensInNewTab, - title: label && stripHTML( label ), - } ), - [ label, opensInNewTab, url ] - ); + // const link = useMemo( + // () => ( { + // url, + // opensInNewTab, + // title: label && stripHTML( label ), + // } ), + // [ label, opensInNewTab, url ] + // ); + + // ...( newUrl && { url: encodeURI( safeDecodeURI( newUrl ) ) } ), + // ...( label && { label } ), + // ...( undefined !== opensInNewTab && { opensInNewTab } ), + // ...( id && Number.isInteger( id ) && { id } ), + // ...( kind && { kind } ), + // ...( type && type !== 'URL' && { type } ), + + const { toLink, toData } = getLinkValueTransforms( { + url: { + dataKey: 'url', + toData: ( value ) => encodeURI( safeDecodeURI( value ) ), + }, + title: { + dataKey: 'label', + toLink: ( value ) => stripHTML( value ), + toData: ( + newLabel = '', + { url: newUrl = '' }, + { label: originalLabel = '' } + ) => { + const newLabelWithoutHttp = newLabel.replace( + /http(s?):\/\//gi, + '' + ); + const newUrlWithoutHttp = newUrl.replace( + /http(s?):\/\//gi, + '' + ); + + const useNewLabel = + newLabel && + newLabel !== originalLabel && + // LinkControl without the title field relies + // on the check below. Specifically, it assumes that + // the URL is the same as a title. + // This logic a) looks suspicious and b) should really + // live in the LinkControl and not here. It's a great + // candidate for future refactoring. + newLabelWithoutHttp !== newUrlWithoutHttp; + + // Unfortunately this causes the escaping model to be inverted. + // The escaped content is stored in the block attributes (and ultimately in the database), + // and then the raw data is "recovered" when outputting into the DOM. + // It would be preferable to store the **raw** data in the block attributes and escape it in JS. + // Why? Because there isn't one way to escape data. Depending on the context, you need to do + // different transforms. It doesn't make sense to me to choose one of them for the purposes of storage. + // See also: + // - https://github.com/WordPress/gutenberg/pull/41063 + // - https://github.com/WordPress/gutenberg/pull/18617. + return useNewLabel + ? escapeHTML( newLabel ) + : originalLabel || escapeHTML( newUrlWithoutHttp ); + }, + }, + opensInNewTab: 'opensInNewTab', + } ); + + const link = toLink( { + url, + label, + opensInNewTab, + } ); + + function handleOnChange( newValue ) { + // Todo: `title` is undefined when adding a new link. Why???? + return props.onChange( toData( newValue ) ); + } return ( Date: Fri, 6 Oct 2023 11:14:05 +0100 Subject: [PATCH 12/12] Improve test descriptions --- .../src/components/link-control/test/link-value-transforms.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/block-editor/src/components/link-control/test/link-value-transforms.js b/packages/block-editor/src/components/link-control/test/link-value-transforms.js index 4a073e0e02585..ea0b0a469a57c 100644 --- a/packages/block-editor/src/components/link-control/test/link-value-transforms.js +++ b/packages/block-editor/src/components/link-control/test/link-value-transforms.js @@ -50,7 +50,7 @@ const mapping = { }, }; -describe( 'buildLinkValueFromData', () => { +describe( 'building a link value from data', () => { it.each( [ [ { @@ -117,7 +117,7 @@ describe( 'buildLinkValueFromData', () => { } ); } ); -describe( 'buildDataFromLinkValue', () => { +describe( 'building data from a link value', () => { it( 'build a valid data object from supplied link value mapping', () => { const linkValue = { url: 'https://www.wordpress.org',