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.
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.
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
Experimental Link creation interface #17846
Experimental Link creation interface #17846
Changes from 71 commits
8dbc2bd
ee008be
e685003
70049ad
98e6aa5
85a6317
8ec1488
140cc90
d64608e
30e0eb6
53ce517
b53c13c
6d7259a
4f32429
4971e30
4aea19c
d554b65
26c4c1b
e778a04
3ca482e
aec9f39
70bd877
58220f8
5eeb819
d355133
2044065
b256d55
b55f557
2133fb1
70c5c04
4a031ed
5b5164c
664daad
9780959
5c3bf35
c962dc6
d4c0a70
8cff4c2
b4a3f66
fbe49e7
fa0dd36
f5a7e65
a88826e
ec9e2ad
11ca79f
779d440
4941bb8
452ec55
5484a7c
37de5d9
ba32148
33467f3
553be99
ad46c87
26c292f
17be2f8
13f5ce1
556088a
1cdd939
2588b9e
a065936
98836fd
5254015
811ad11
9cd6d18
bdb6217
d0a348b
da212f0
19d5e64
548279b
a844800
fd3a6ef
b314c49
5aeb531
1a7c285
63201c9
4918184
cd29ab5
2d8befb
d8895d5
2413016
e3042c8
0a9d558
89e92fb
ff57160
48e5f44
4e811db
43c30b2
158ea3e
4981a71
f1c54a6
797fd6c
e5e44e6
5ba4b65
d5abad2
5c1ec22
4d5b455
b8a01bd
7df9aa5
b6edb73
b435b49
8c5ed79
1fffe8e
1c1614d
0cdf706
2f80347
b93a0db
5b6715d
280db32
ba985eb
7d35791
e7585b8
8450bb1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: some of these handlers (
onInputChange
,resetInput
,onStartEditing
) could be inlined, my opinion is that it makes the code a bit more readable. Just my opinion though, so up to you if you change it.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it might be tidier if all the code from
handleURLSearch
andhandleEntitySearch
were defined ingetSearchHandler
. There seems to be a lot of logic aroundisURL
,couldBeURL
,value.includes( 'www.' )
that could be streamlined by moving all the code to be in the same place.Other than that, this could use an array spread:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getSearchHandler
is an 'effect' according to the comment, butresetInput
isn't. Both seem to useuseCallback
. The name could possibly be changed as it doesn't return asearchHandler
from what I can tell, but thesearchResults
instead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For whatever reason, Gutenberg doesn't tend to use the
--modifier
BEM syntax. A simple class likeis-editing
is fine.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: There's
LinkControlSearchItem
andLinkControlInputSearch
. I think it'd be more consistent if this one were namedLinkControlSearchInput
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's seems like a lot of code duplication that
stopPropagation
needs to be handled whenURLInput
already handles it for its normal search results.The extra API being added to
URLInput
(renderSuggestions
,onKeyDown
,onKeyPress
) is something I'm concerned about. A lot of it is code that is already defined internally inURLInput
. At the very least these props need to be all marked as experimental.I also haven't seen any use case mentioning the need to make search results extensible in this way.
Even if it needs to be extensible, I feel like the way it's implemented could be more elegant, like a component that gets passed to
URLInput
(after allrenderSuggestions
renders in exactly the same part of the render tree as the standard suggestions). Then it'd be possible to provide some good options out of the box, e.g.:or
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not really. The
<URLInput />
doesn't block the propagation of the events and depending on the context when it's used the popover can disappear (apparently) spontaneously. You can see how it's handled in #17986 PR, here for instance.There are some specific events that need to be handled in the
<URLInput />
component (like it already does). It's nice to see how the caret goes at the beginning or at the end when the user pressUP
orDOWN
.But handling the
LEFT
andRIGHT
keys maybe need to be handled outside of the<URLInput />
, since this is an special case where the component is used into a NavigationMenuItem , and we need to block the shifting to the next ones.I think adding the
__experimental
prefix toonKeyPress
oronKeyDown
could add more confusion. Why not simply passing the props and make them optional?or something like that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...but I suggest handling it by the component which consumes it. For instance, in the NavigationMenuItem:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me show how we are handling this in the
[draft]
PR.<LinkControl />
component.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agree we shouldn't handle onKeyDown from the URLInput, in the first place because stopping propagation for those particular keys is not necessary in all the use cases of URLInput (for instance, the Button block doesn't need it) and second because URLInput already has its own onKeyDown handler and passing in a second one makes the code confusing and hard to read.
However, I don't we think should be handling it in the Item component either. I added that bit in my PR for the sake of making things work, but am expecting it to be refactored at some point once we've decided on how this new link component should behave.
This keyDown handler should only be needed in cases where we have a URLPopover inside the block toolbar, and because we now have multiple instances of this - Image block, any block with RichText in it, and now MenuItem - it might be a good time to build a toolbar-specific URLInput wrapper that handles all these gotchas in one place.
(There was a bit of discussion about that on my PR, @draganescu has run into the same issue with the Media flow component.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@retrofox
URLInput
has been in use for a long time in popovers without those issues. The component does stop propagation of the events that are relevant to it:gutenberg/packages/block-editor/src/components/url-input/index.js
Lines 175 to 208 in 6cf43ac
Other events do seem to be stopped at the form/popover level where
URLPopover.LinkEditor
is implemented. I do agree that there might be better ways to do this as it's a lot of code duplication. The issue would probably be that URLInput can be used both in and outside of a popover, so it shouldn't be handling this functionality internally (beyondUP
,DOWN
,TAB
,ENTER
, which apply to both use cases).I'm not sure I see why it would add more confusion. It signals that they're not (yet) for use externally, which is the truth seeing as we don't seem to have agreement on how this should be implemented. At the moment it would be a pragmatic way to get this PR shipped.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far 😆. It happens when we introduce new features. Simply happens.
Yes, it's true. I'm just wondering why not just pass the
onKeyPress
to the<URLInput />
component, making it optional defining anoop
function as default.To me, it is the opposite. Improves the API instead of adding a new wrapper element just for subscribing to the event. So the options are:
<URLInput />
just to catch the event propagation.onKeyPress
property of the<URLInput >
It's just part of the workflow, right? Totally agree to go for whatever we want and go ahead not blocking us. Personally, I prefer the second one but it's up to us. I've already added another comment about this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is
<URLInput />
handling theonKeyPress
event? It doesn't seem to do.I doubt because it already tries to bind the function in the current implementation of
<NavigationMenuItem />
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
19d5e64 handles the
onKeyPress
event of the<URLInput />
component.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same logic as for the keyDown I commented on above: this keyPress handler is only needed for popovers inside the toolbar so if we create a toolbar URL wrapper we should add it in there instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, it's handled by the
<form />
element, right? the Form subscribes to theonKeyPress
and then stops its propagation when happens.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this needed when there's already a
showSuggestions
prop?fetchLinkSuggestions
beingundefined
could also indicate to the component that suggestions shouldn't be handled.