[WIP] Provide standardised tools for mapping Data to Link attributes #54791
Conversation
| 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; | ||
| } |
There was a problem hiding this comment.
These two functions can likely be normalized into a single function. Or at least they can be well-named abstractions over that single utility function.
|
Size Change: +263 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
|
Next step here is to try and utilise the new mapping functions inside:
Need PoCs and also to stress test the utils. All new scenarios encountered should be encoded into the tests. |
|
Pinging @bangank36 who did some work around building link attributes in #54110. I wonder if you feel these functions could replace |
|
@getdave Thank you for the ping!
I like this behavior, I used to be confused with the conversion of
With the current implementation, I think these functions can only work well with 1:1 mapping, while |
This is a great example. Thank you. I'll see if I can refactor to allow for this. |
packages/block-editor/src/components/link-control/test/link-value-transforms.js
Show resolved
Hide resolved
packages/block-editor/src/components/link-control/test/link-value-transforms.js
Show resolved
Hide resolved
getdave
force-pushed
the
try/standardise-link-to-data-mapping
branch
from
297f0f7
to
4a34767
Compare
| updatedRel = updatedRel?.replace( relRegex, '' ).trim(); | ||
| } | ||
|
|
||
| // Handle setting rel based on opensInNewTab setting. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@getdave I have thought about this, can we somehow define util methods for the mapping object
- Function that returns mapping object
const mapping = ( () => {
const utils = {
method1: ()=>{},
...
}
return {
'url': 'href'
'nofollow': ()=>{ utils.method1(); }
}
} )();
- 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
There was a problem hiding this comment.
I made a PR for demo this #55143
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 ( value && currentVal ) { | ||
| return `${ currentVal } nofollow`; | ||
| } else if ( value ) { | ||
| return 'nofollow'; | ||
| } |
There was a problem hiding this comment.
This type of "append to existing" operation feels like it could be fairly common. Should we look to provide some kind of utility to make this simpler and avoid boilerplate?
| const linkValue = useMemo( | ||
| () => ( { url, opensInNewTab, nofollow } ), | ||
| [ url, opensInNewTab, nofollow ] | ||
| const linkValue = buildLinkValueFromData( |
There was a problem hiding this comment.
There was a problem hiding this comment.
I think we just need to wrap buildLinkValueFromData in a useCallback and then use it inside of the useMemo.
getdave
added
the
[Feature] Link Editing
|
Flaky tests detected in 2eb7259. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6430348972
|
|
The next objective here is to modify the Nav Link block to utilise the same utility functions and allow us to reduce the complexity inside of |
getdave
force-pushed
the
try/standardise-link-to-data-mapping
branch
from
9ac0b0a
to
63a5b19
Compare
| } ); | ||
|
|
||
| describe( 'building data from a link value', () => { | ||
| it( 'build a valid data object from supplied link value mapping', () => { |
There was a problem hiding this comment.
We'll need to break this into discreet tests asserting on the behaviour. We should do this before things get overly complex 😓
| // ...( kind && { kind } ), | ||
| // ...( type && type !== 'URL' && { type } ), | ||
|
|
||
| const { toLink, toData } = getLinkValueTransforms( { |
There was a problem hiding this comment.
Very much a WIP. Does not work...yet
| { url: newUrl = '' }, | ||
| { label: originalLabel = '' } |
There was a problem hiding this comment.
Whilst terse, this does require the reader to understand the API of the toData reducer.
Instead let's define these as well named arguments (e.g. fullLinkValue, currentDataValue) and then destructure from them.
| url, | ||
| label, | ||
| opensInNewTab, |
There was a problem hiding this comment.
We should probably have this as a well renamed const and define it wherever we access these values further up the component.
| postType: 'post', | ||
| id: 123, | ||
| linkTarget: '_blank', | ||
| linkRel: 'nofollow sponsored', |
There was a problem hiding this comment.
@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...
There was a problem hiding this comment.
The failed test is found when running unit test against link-value-transforms-with-utils.js
There was a problem hiding this comment.
@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'
There was a problem hiding this comment.
It looks like I don't have privilege to push commit to this PR, therefore I opened new PR to propose my approach #55143 on the mapping object
Instead of using string concatenation, I used Set to ensure unique values and simplify management
How many PRs have you merged into the Gutenberg repo? If it's 3 or more I can request that you be given repo rights.
I took a quick look at the PR - thank you. It seems like the change is to the test files right? If so I'm not sure I agree we should have this level of abstraction within a test. In general, I'd prefer to aim for everything to be concrete even if that means lots of duplication and manual string concat. Why? Because by keeping tests simple we avoid writing test code that itself needs testing therefore reducing the possibility of false positives in tests due to bugs in test code. Also it is easier for those unfamiliar with the code under test to perceive and work with the tests. You have spotted some errors above which we should fix. Thank you 🙇 |
There are 9 thus far
No, it's the suggestion for changing the implementation of the mapping for ButtonEditor, as explained here #54791 (comment)
I have some more thoughts on this... #54791 (comment), I think the test is correct |
I made a request on the WP Slack instance (sign up required). |
Thank you! |
What?
Provides a set of standardised functions to map stored data attributes into valid
LinkControlvalueattributes.Why?
There are numerous places in Core (and beyond) where some data must be mapped to the
LinkControl'svalueobject. This process must also happen in reverse whenLinkControlonChangeis called.There are several implementations and each one has it's own quirks, is complex to reason about/change, and lacks automated test coverage.
In addition there are no extension points for how the data should be mapped/transformed, meaning that it is not possible to add additional settings/controls to the various Link UIs used throughtout the editor. Chief amongst these is the RichText implementation which 3rd party extenders regularly want to modify to add additional settings.
This PR seeks to resolve this by providing consumers with a standardised (largely declarative) API for describing how data objects should be mapped to
LinkControlvalues.It also paves the way for the possibility of making the "map" objects passed to these functions extensible. This would allow 3rd party extenders to add additional controls to the Link UI within the editor without the current process which requires re-registering the
core/linkformat from scratch!How?
Instead of imperitively coding how the link value should be decoded/encoded, instead we provide utility functions which act over standardised "map" objects which describe how the
LinkControl'svaluefields map to the given data object.By default the mapping works 1:1 based on string equivalence. So in the case below the
urlattribute of theLinkControlobject is mapped to thehreffield of the data object. The data object might be block attributes or any other form of storage.For more complex examples, consumers may define more complex transformations by way of
toLinkandtoDatawhich describe how the different values should be transformed.Below is a simplified example of this:
Now the values can be passed to either transformation function as required and the result will be the desired object ready to either:
LinkControlas thevaluepropsetAttributes())Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast