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

[WIP] Provide standardised tools for mapping Data to Link attributes #54791

Draft
wants to merge 12 commits into
base: trunk
Choose a base branch
from
Draft

[WIP] Provide standardised tools for mapping Data to Link attributes #54791

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
6271874
Initial mapping functions and tests
getdave Sep 20, 2023
5a9914d
Lay foundation for more complex tests
getdave Sep 20, 2023
aa811f8
Export data mapping utils
getdave Sep 20, 2023
13071ae
Pass full link object as secon param for full utility
getdave Sep 29, 2023
e8526cd
Re-implement link updating using standardised utils
getdave Sep 29, 2023
1edac52
Ensure http is prepended to any stored URL
getdave Sep 29, 2023
b98a8a9
Export single build function
getdave Oct 6, 2023
49cf4fa
Fix tests by passing full data value to reducer
getdave Oct 6, 2023
855c646
Update Button implementation to match API
getdave Oct 6, 2023
13be3c6
Add ability to selectively omit transforms
getdave Oct 6, 2023
63a5b19
WIP start updating Nav Link block transforms
getdave Oct 6, 2023
2eb7259
Improve test descriptions
getdave Oct 6, 2023
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
4 changes: 4 additions & 0 deletions packages/block-editor/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,10 @@ _Returns_

- `string`: Gradient value.

### getLinkValueTransforms

Undocumented declaration.

### getPxFromCssUnit

Returns the px value of a cssUnit. The memoized version of getPxFromCssUnit;
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +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 { 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';
Expand Down
45 changes: 45 additions & 0 deletions packages/block-editor/src/components/link-control/link-value-transforms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
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 if ( isCallable( valueGetter.toLink ) ) {
linkValue[ attributeName ] = valueGetter.toLink(
data[ valueGetter.dataKey ],
data
);
} else {
linkValue[ attributeName ] = data[ valueGetter.dataKey ];
}
}
return linkValue;
}

function buildDataFromLinkValue( linkValue, mapping ) {
const data = {};
for ( const [ attributeName, valueGetter ] of Object.entries( mapping ) ) {
if ( typeof valueGetter === 'string' ) {
data[ valueGetter ] = linkValue[ attributeName ];
} else if ( isCallable( valueGetter.toData ) ) {
data[ valueGetter.dataKey ] = valueGetter.toData(
linkValue[ attributeName ],
linkValue,
data
);
} else {
data[ valueGetter.dataKey ] = linkValue[ attributeName ];
}
}
return data;
}

export default function getLinkValueTransforms( mapping ) {
return {
toLink: ( data ) => buildLinkValueFromData( data, mapping ),
toData: ( linkValue ) => buildDataFromLinkValue( linkValue, mapping ),
};
}
161 changes: 161 additions & 0 deletions packages/block-editor/src/components/link-control/test/link-value-transforms.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/**
* Internal dependencies
*/
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
* 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',
getdave marked this conversation as resolved.
Show resolved Hide resolved
type: 'postType',
id: 'id',
opensInNewTab: {
dataKey: 'linkTarget',
toLink: ( value ) => value === '_blank',
toData: ( value ) => ( value ? '_blank' : undefined ),
},
noFollow: {
dataKey: 'linkRel',
getdave marked this conversation as resolved.
Show resolved Hide resolved
toLink: ( value ) => value.includes( 'nofollow' ),
toData: ( value, _, { linkRel: currentLinkRel } ) => {
// if the value is truthy and the current value is set
// then append otherwise just add the value
if ( value && currentLinkRel ) {
return `${ currentLinkRel } nofollow`;
} else if ( value ) {
return 'nofollow';
}
},
},
sponsored: {
dataKey: 'linkRel',
toLink: ( value ) => value.includes( 'sponsored' ),
toData: ( value, _, { linkRel: currentLinkRel } ) => {
// if the value is truthy and the current value is set
// then append otherwise just add the value
if ( value && currentLinkRel ) {
return `${ currentLinkRel } sponsored`;
} else if ( value ) {
return 'sponsored';
}
},
},
};

describe( 'building a link value from data', () => {
it.each( [
[
{
href: 'https://www.wordpress.org',
postType: 'post',
id: 123,
linkTarget: '_blank',
linkRel: 'nofollow noopenner sponsored',
keyToIgnore: 'valueToIgnore',
},
{
url: 'https://www.wordpress.org',
type: 'post',
id: 123,
opensInNewTab: true,
noFollow: true,
sponsored: true,
},
],
[
{
href: 'https://www.wordpress.org',
postType: 'post',
id: 123,
linkRel: 'sponsored neyfollow',
},
{
url: 'https://www.wordpress.org',
type: 'post',
id: 123,
opensInNewTab: false,
noFollow: false,
sponsored: true,
},
],
] )(
'build a valid link value from supplied data mapping',
( data, expected ) => {
const { toLink } = getLinkValueTransforms( mapping );

const linkValue = toLink( data );

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( 'building data from a link value', () => {
it( 'build a valid data object from supplied link value mapping', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'll need to break this into discreet tests asserting on the behaviour. We should do this before things get overly complex 😓

const linkValue = {
url: 'https://www.wordpress.org',
type: 'post',
id: 123,
opensInNewTab: true,
noFollow: true,
sponsored: true,
};

const { toData } = getLinkValueTransforms( mapping );
const data = toData( linkValue );

expect( data ).toEqual( {
href: 'https://www.wordpress.org',
postType: 'post',
id: 123,
linkTarget: '_blank',
linkRel: 'nofollow sponsored',
Copy link
Contributor

Choose a reason for hiding this comment

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

@getdave I think the expected linkRel should be noopener noreferer nofollow sponsored because the linkValue is

opensInNewTab: true,
noFollow: true,
sponsored: true,

This test case is passed with incorrect value...

Copy link
Contributor

Choose a reason for hiding this comment

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

The failed test is found when running unit test against link-value-transforms-with-utils.js

Copy link
Contributor

Choose a reason for hiding this comment

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

@getdave but I think this is expected with the current mapping object in the test file, because the default mapping is 1:1, therefore nofollow and sponsored linked with linkRel while opensInNewTab is not, so the expected linkRel is correct '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',
} );
} );
} );
94 changes: 72 additions & 22 deletions packages/block-library/src/button/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -32,12 +31,14 @@ import {
__experimentalUseColorProps as useColorProps,
__experimentalGetSpacingClassesAndStyles as useSpacingProps,
__experimentalLinkControl as LinkControl,
getLinkValueTransforms,
__experimentalGetElementClassName,
} from '@wordpress/block-editor';
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,
Expand Down Expand Up @@ -133,10 +134,67 @@ 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 = {
getdave marked this conversation as resolved.
Show resolved Hide resolved
url: {
dataKey: 'url',
toData: ( value ) => prependHTTP( value ),
},
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 } ) => {
getdave marked this conversation as resolved.
Show resolved Hide resolved
// "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 = '';
getdave marked this conversation as resolved.
Show resolved Hide resolved

// 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.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What do we think about handling another attribute in this handler? If we don't do it like this we will need some means within toData to trigger mutations in other parts of the data tree.

Whilst the approach here isn't one I love, it feels less complex to the consumer than any alternative I can conceive of right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also this might cause unexpected conditions because what if opensInNewTab were processed after nofollow? That might mean that opensInNewTab value might still represent the old value as it has yet to be processed.

We need a test for such a scenario to make it concrete and then we can consider how best to resolve it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Possibly a "dependencies" idea, whereby properties can declare that they have dependencies on other properties. Then the final value for the property is only calculated once the other property has been processed.

Copy link
Contributor

@bangank36 bangank36 Oct 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@getdave I have thought about this, can we somehow define util methods for the mapping object

  1. Function that returns mapping object
const mapping = ( () => {
    const utils = {
        method1: ()=>{},
        ...
    }
    return {
       'url': 'href'
       'nofollow': ()=>{ utils.method1(); } 
   }
} )();
  1. Object with utilities methods
const mapping = {
    'url': 'href',
    'utils': {
        method1: ()=>{},
        ...
    }
}

With this approach, we may need to exclude the prop name from the link-value-transforms methods, since it does not have standard toLink and toData methods like normal props

Copy link
Contributor

Choose a reason for hiding this comment

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

I made a PR for demo this #55143

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've also been thinking about ths concept of utility functions. I think on balance it might be easier to simply allow consumers to provide their own utilities as needed.

For example much of the transforms could be handled with a functional approach using something like compose to compose a set of transformations.

I think the most common one will be "if this value is a string then concat to an existing string if it exists or otherwise just use the string as the new value". Something like Ramda Concat but that can handle undefined would be what we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I've just realised the concept in your PR 👍

So we could export an additional helper from link-value-transforms called mapGenerator where we could expose utils for mapping.

If I've understood this would accept a mapping object and provide callbacks to the map to allow the consumer to perform typical data operations.

This would also be completely optional.

I think it's clever. I think we should

  • implement a basic version of this (my) PR
  • come back to your idea of mapping generators in a follow up.

I really like the idea but I think it may overcomplicate this initial PR. But I would definitely like to explore it.

What do you think?

Copy link
Contributor

@bangank36 bangank36 Oct 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This would also be completely optional.

Yes, it is
The PR #55143 should be a commit to modify the ButtonEdit mapping, but because of the lack of write privilege, I used it as a separate proposal with its own test for better explanation

I think on balance it might be easier to simply allow consumers to provide their own utilities as needed.

The same explain as above, the PR was meant for ButtonEdit (as consumer) with it own utils

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;
},
},
};

const { toLink, toData } = getLinkValueTransforms(
linkValueAttrsToDataMapping
);

function startEditing( event ) {
event.preventDefault();
setIsEditingURL( true );
Expand All @@ -157,12 +215,15 @@ 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 = toLink( {
url,
linkTarget,
rel,
} );
// NOT NOT MERGE WITHOUT RE-INTSTAINTG THIS MEMOIZATION

return (
<>
Expand Down Expand Up @@ -251,19 +312,8 @@ function ButtonEdit( props ) {
>
<LinkControl
value={ linkValue }
onChange={ ( {
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} ) =>
setAttributes(
getUpdatedLinkAttributes( {
rel,
url: newURL,
opensInNewTab: newOpensInNewTab,
nofollow: newNofollow,
} )
)
onChange={ ( newLinkValue ) =>
setAttributes( toData( newLinkValue ) )
}
onRemove={ () => {
unlink();
Expand Down
6 changes: 1 addition & 5 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 @@ -575,11 +575,7 @@ export default function NavigationLinkEdit( {
anchor={ popoverAnchor }
onRemove={ removeLink }
onChange={ ( updatedValue ) => {
updateAttributes(
updatedValue,
setAttributes,
attributes
);
setAttributes( updatedValue );
} }
/>
) }
Expand Down
Loading