Simplify api for link UI abstraction to use a single prop for the value #46189
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
getdave
added
[Type] Code Quality
getdave
commented
Nov 30, 2022
Comment on lines
-55
to
-59
| const link = { | ||
| url: insertedBlockAttributes.url, | ||
| opensInNewTab: insertedBlockAttributes.opensInNewTab, | ||
| title: insertedBlockAttributes.label, | ||
| }; |
There was a problem hiding this comment.
Previously you needed to know about the structure of this object. Now you just pass all the attributes.
getdave
commented
Nov 30, 2022
| const link = { | ||
| url: props.link.url, | ||
| opensInNewTab: props.link.opensInNewTab, | ||
| title: props.link.label && navStripHTML( props.link.label ), |
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
I created #46193 to track.
There was a problem hiding this comment.
makes sense, thanks!
There was a problem hiding this comment.
I found this comment which suggests moving to the stripHTML would be ok
Should we create an issue for this too?
getdave
force-pushed
the
try/simplify-api-for-link-ui-abstraction
branch
from
ae12902 to
99a7294
Compare
|
Size Change: +8 B (0%) Total Size: 1.32 MB
ℹ️ View Unchanged
|
MaggieCabrera
approved these changes
Nov 30, 2022
scruffian
reviewed
Nov 30, 2022
| @@ -146,6 +162,12 @@ export function LinkUI( props ) { | |||
| }; | |||
| } | |||
|
|
|||
| const link = { | |||
There was a problem hiding this comment.
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.
mpkelly
pushed a commit
to mpkelly/gutenberg
that referenced
this pull request
Dec 7, 2022
…ue (WordPress#46189) * Refactor to link prop for all link related data * Apply same refactor to originalk code in Nav Link block
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Simplifies the API for
<LinkUI>across both instances to have a single prop for the values related to a link.Why?
In #46031 we created an abstaction for the Link UI in Nav Link blocks so we could easily reuse (copy/paste) it in the offcanvas experiment.
However the API is overly complex as it requires the consumer to knowing how to deconstruct the values required by the underlying
<LinkControl>.With this change the consumer just passes all the props from the Nav block attributes and the component sorts it out to be consumabnle by the underlying LinKControl.
How?
See above.
Testing Instructions
The main test is checking that both Nav Link block in canvas and offcanvas LinkUI's behave as on
trunk.Testing Instructions for Keyboard
Screenshots or screencast