Add featured transforms in Link Control for Navigation Link items. #35857
Conversation
|
Size Change: +782 B (0%) Total Size: 1.08 MB
ℹ️ View Unchanged
|
tellthemachines
added
the
[Block] Navigation Link
|
Took it for a quick spin, and seeing these nice waiting-to-be-styled buttons in the transform dialog: Honestly with some nice styles here, I imagine this could be pretty decent! If you feel happy about the code here, and @getdave is okay with it as well, I'd be happy to push some CSS tweaks to shine this one up! (Though I'd appreciate help adding the right icons and labels to the buttons!) |
| @@ -292,6 +293,7 @@ function LinkControl( { | |||
| ) } | |||
| </div> | |||
| ) } | |||
| { children } | |||
There was a problem hiding this comment.
My concern is, whilst this is expedient, it does lock us into a particular location for children.
I wonder whether instead we have dedicated locations for rendering additional UI inside LinkControl.
So in this case we want to add content to the bottom of the control so a property for this would be called renderControlBottom (or something). We could treat as a render prop and that would allow you to optionally pass content to be rendered there. Treating as a render prop would also give you the ability to pass some of the internal state of link control to the inserted component.
If later on we decide we need to insert content into another location we can add renderControlTop or whatever.
There was a problem hiding this comment.
Changed this to use render prop! I hate naming things so conveniently used your suggestion 😄
|
Updated to add the block icon and proper title, as well as use the Button component which gives us at least a reset of user agent button styles. If we want these buttons to look like the inserter ones, I think we only need to move the icon on top of the text and add a bit of padding. I'm thinking we also need a title for this section. Shall we go with "Or transform to block" as @jasmussen's design shows? A few questions to consider: Currently the three items on display are hardcoded. Will we ever want to have different blocks appearing? I asked this already on the issue, but adding it here again for visibility: do we want these transforms to always display in the link control for nav links, or do we want to hide them when different block types are already available from the quick inserter? Also, do we want to hide these when a link is already set in the block? E.g. |
|
This is looking good!
I'd like to add a separator, tweak the padding a little bit, and tinker with whether stacked buttons are better or not, like this:
Mind if I push to the branch? Speaking of, should we change the button defaults to logo, title, search? Depends a bit on whether we can land #35688 soon, if we can then Search might be a better experience than Social Links 🤔
My instinct would be to hide these, yes. |
|
Actually I held off on pushing my changes because it felt like I was adding a tone of bespoke CSS in arbitrary places. So I'll hold off for now, and let me know if/how/where this would be appropriate. I managed to get it to look like this: That moves the "Open in new tab" to a toggle you can flip after the fact, not at the initial step. It assumes this markup snippet: I then added these pieces of CSS. Buttons: Button container — I guess we might have a counterpart among the new components? Subheading, basically the same as this one, so it should probably be reused: General container: As we look to the new components to absorb much of this bespoke CSS, it just feels wrong to add it. At the same time, it might be the most direct path to a solution. What do you think? |
|
Thanks for sharing those styles @jasmussen ! I incorporated them in the lastest changeset and it should be looking more or less as expected now.
We're already using
There's no ready-made way to do this. "Open in new tab" is a default setting in LinkControl, and currently there's no way of setting it conditionally in that component. Ideally we'd be able to pass a prop to LinkControl to configure when to render "Open in new tab", but as that involves changing the LinkControl API, we should do it as a separate task. Cc. @getdave |
| inserterItems: getInserterItems( | ||
| getBlockRootClientId( clientId ) | ||
| ), |
There was a problem hiding this comment.
I'm wondering if it should use a selector like getBlockTransformItems.
| * Add transforms to Link Control | ||
| */ | ||
|
|
||
| function LinkControlTransforms( { block, transforms } ) { |
There was a problem hiding this comment.
Could this one be un-nested? Or alternatively wrapped in useCallback?
In the past, I've seen a component defined within another component in this way cause unexpected behaviors in React. The reconciler can treat it as a new component on every render because the function reference changes each time.
I actually haven't noticed any issues in testing, but I still think it's a pattern worth avoiding.
|
|
||
| function LinkControlTransforms( { block, transforms } ) { | ||
| return ( | ||
| <div className="link-control-transform"> |
There was a problem hiding this comment.
Not sure what the right class name would be. It should maybe use the wp-block-navigation-link class if its defined with the component. It could get quite wordy.
There was a problem hiding this comment.
I'm seeing this as a micro-component that happens to be used in navigation link, but could potentially be used in any other block that has LinkControl as its main feature.
| const featuredBlocks = [ | ||
| 'core/site-logo', | ||
| 'core/social-links', | ||
| 'core/search', | ||
| ]; | ||
| const featuredTransforms = inserterItems.filter( ( item ) => { | ||
| return featuredBlocks.includes( item.name ); | ||
| } ); |
There was a problem hiding this comment.
Maybe not needed for a first draft, but if these hard-coded blocks are unregistered by a site maintainer or plugin, it could be worth falling back to other available transforms.
There was a problem hiding this comment.
Hmm, good point, what's the best way to check the blocks are registered?
|
Incredible work, and it's coming together so fast, this looks very close to me: I also appreciate that you used the flex components to make this happen, rather than the bespoke CSS I suggested! I solved that in the pseudo CSS by adding It appears partially intentional from the component side of things, there's even a faux "gap" type margin:
But the net result is that the combination of space-between, gap, and flex-basis: 33%; isn't really working to make the three items equally weighted. I could probably dive in and override some of the component CSS, but that also feels like the wrong approach. Any idea of a good fix? |
Oh, that's weird, I can't reproduce it. It should just work, because it's using the Flex component. Though we might have to change that, because of:
The problem with the Flex component is that FlexItem wraps an element around our buttons, so instead of having which would allow us to style the buttons as flex children, we end up with and because Flex is set up to use CSS-in-JS, we can't just hook some extra CSS to its classnames. We can pass the styles inline to FlexItem, but at that point it's easier (and much less messy) to not use Flex at all, and attach the CSS directly to our LinkControlTransforms markup. Flex is still marked as experimental and if it is to be useful at all as a component it needs a fair bit of improvement. Not forcing a wrapper around the FlexItems would be a good start, as well as allowing customisation of individual item styles. To be honest I'm not sold on the idea of a component with the sole function of adding three lines of CSS to a div: it doesn't seem to solve any meaningful problem, and it makes the DX horrible (code bloat, wrappers around wrappers, having to hunt through multiple files to find where a style is declared). But I'm probably missing some context around this 🤷 Tl,dr: I think our best bet is to avoid using Flex for now, and add the styles manually. |
tellthemachines
force-pushed
the
try/transforms-in-link-control
branch
from
14a8530 to
9e23d7e
Compare
Yeah, seems like FlexItem should have an <FlexItem as={ Button }>But it doesn't right now. It always renders as a |
Hmm, adding margin or padding to the buttons or to their containers triggers a scrollbar on the popover 😕 |
|
Glad this worked out ❤️ |
andrewserong
changed the title
Description
Partially addresses #34041
Adds three transform buttons inside the link control for Navigation Links. The blocks to transform to are hardcoded; they're a selection of the options that might be most useful when expanding the Navigation to include more than just links: Search, Site Logo and Social Icons.
Adds a
renderControlBottomrender prop to LinkControl that allows adding extra controls to the bottom of the link popover.How has this been tested?
In the Navigation block, add a new link. Check that block transforms are showing in the link popover.
Open the link popover for an existing navigation link that has a URL set. Transforms should not display.
Link popover should be unchanged for regular links in paragraphs etc..
Screenshots
Types of changes
New feature (non-breaking change which adds functionality)
Checklist:
*.native.jsfiles for terms that need renaming or removal).