[LinkControl] Extract reusable parts #23869
Conversation
|
Size Change: +594 B (0%) Total Size: 1.15 MB
ℹ️ View Unchanged
|
adamziel
force-pushed
the
update/reusable-link-control-parts
branch
from
c99cdbd to
880fcbf
Compare
adamziel
added
[Block] Navigation
adamziel
requested review from
ajitbohra,
ellatrix,
Soean,
talldan and
youknowriad
as code owners
|
Do we really need to split it into separate components or should this component be replaced entirely by what the issue is proposing instead? It seems to indicate that it's the whole component that needs to be changed for all these places. My main concern is that by splitting into smaller components, we'll perpetuate the inconsistencies. For example how is this new component different from URLInput? |
@youknowriad That's the endgame here, I was concerned about doing it all in a single PR as it would likely touch 40+ files and sit in the review queue forever. My plan ATM is to 1. Merge this change 2. Phase out the LinkControl from every place it's used 3. Remove LinkControl entirely. What do you think? |
Not sure I understand, why not just rename |
|
@youknowriad sure, that works too, the point being getting rid of the current top-level implementation of |
| import { useEffect, useState, useRef } from '@wordpress/element'; | ||
|
|
||
| export default function useCreatePage( customCreatePage = null ) { | ||
| const { saveEntityRecord } = useDispatch( 'core' ); |
There was a problem hiding this comment.
This can't be used in "block-editor". Anything that has direct relationship with WP API can't be used as the block-editor package is a client only package agnostic to WP context.
There was a problem hiding this comment.
@youknowriad what do you think would be the right package for this code to live? I can't quite fit this into any existing one. Are these two hooks (useSearchHandler and useCreatePage) enough to justify creating a new package?
There was a problem hiding this comment.
It seems like useSearchHandler still fits the block-library as it depends on the editor setting, not on any WP-specific logic. It seems like the same trick should work for the useCreatePage - let me try it.
There was a problem hiding this comment.
It would work but ATM the go-to solution for these things is copying and pasting the code to every package where the editor is initialized. I don't like that so I went back to passing the logic from the navigation-block/edit which ATM is the only consumer of that feature. Let's absolutely revisit later when we talk about the higher level direction for these things.
There was a problem hiding this comment.
Do you think there's room for a completely generic LinkControl component inside the @wordpress/components package? (no direct API, no block-editor dependency, just a UI component with potentially some config)
This could allow us to have a storybook story and built it in isolation.
|
@youknowriad I think so! I am checking what would it take. LinkControl depends on URLInput at the moment so it would have to be moved there as well. Or would it? Maybe they don't need to be two separate components. It seems like URLInput is an input that supports fetching and rendering arbitrary suggestions, while LinkControl/LinkControlSearchInput exposes the API to do the same plus a bit more page-specific logic. I think these two could become the same component with some sensible, generic defaults. The only thing that wouldn't move cleanly are |
|
Also, it may be crazy but here's an idea - |
|
Yes, on principle I see this too: a Single LinkControl component in UI components, potentially another one in block-editor to "tie it" to the block editor config and use it everywhere. but that's the theory. As you can see there are multiple places where we use these components slightly differently and the discussion we're having here is one I had in the past with @getdave @aduth and others. So for me, it sounds a bit like we're going on circles: building something, another person comes deciding it's not great, having the same discussion. So, that said, I'm not against, doing another refactor... but it seems that we should ensure that the refactor is actually solving a problem and that we don't end up in a similar position to where we are because in the end-up we find an exception that doesn't fit the scenario. |
|
See these issues #18061 All these issues are related and I want to make sure we don't end up just creating other issues like that. Should we first audit these issues, define a good plan, check what failed in the past and avoid falling in the same trap? cc @getdave would love your thoughts here. I'm personally not very aware of the current state of LinkControl to propose a good path forward. |
|
Thank you for these links! Let's not block this PR - I'll investigate the larger issue and propose a way forward (likely in one of these issues). |
|
With the entity dependency solved, this one is ready for a review. CC @draganescu @noisysocks. I'm investigating what are the options for the larger direction here. |
| */ | ||
| import { ViewerSlot } from './viewer-slot'; | ||
|
|
||
| export default function LinkOverview( { value, onEditClick } ) { |
There was a problem hiding this comment.
This is more like LinkPreview ?
There was a problem hiding this comment.
Good note, I'll update it
packages/block-editor/src/components/link-control/link-overview.js
Outdated
Show resolved
Hide resolved
adamziel
force-pushed
the
update/reusable-link-control-parts
branch
from
2b3b7f1 to
5b030bc
Compare
|
Where can I find an example of using the |
|
@crs1138 There are some examples in the readme file here: https://github.com/WordPress/gutenberg/tree/master/packages/block-editor/src/components/link-control
Your thinking is right, however I'm afraid it's not possible at the moment - not only LinkControl does not support editing of target and rel attributes, but also there's also no way to customize it at the moment. I would very much like to have a component that would handle all these use cases and also provide some extension points. To make LinkControl more flexible, we would either need to add a slot to LinkControl or LinkSettingsDrawer contributing to the surplus of slots, OR export all the building blocks such as |
|
@adamziel Thanks for the link to the component. The examples there show the use of the component without any context of the use thus I ended up in this thread. I was expecting/hoping it'd have a similar functionality as if using the ...
<Button url={url} >
<PlainText
placeholder={ __(`Calling to act`, cvslug) }
value={btnText || `Click`}
onChange={ (newText) => setAttributes({ btnText: newText }) }
/>
</Button>
<URLInputButton
url={ url }
onChange={ (url, post) => setAttributes( { url, btnText: (post && post.title) || `Click here` } ) }
/>
...But all I could achieve with the Btw. interesting read about the challenges and possible solutions regarding the |
|
Apologies @youknowriad i missed the ping. |
Description
This PR refactors a monolithic LinkControl into separate, bite-sized components and hooks that are reusable. This PR paves ground for reusing the
LinkControlSearchInputcomponent in the following interaction designed as a part of #23375:How has this been tested?
Screenshots
Types of changes
Non-breaking change