Skip to content
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

Refactor LinkControl to remove dependency on URLInput #19523

Closed
getdave opened this issue Jan 9, 2020 · 4 comments
Closed

Refactor LinkControl to remove dependency on URLInput #19523

getdave opened this issue Jan 9, 2020 · 4 comments
Assignees
Labels
Needs Technical Feedback Needs testing from a developer perspective. [Package] Block editor /packages/block-editor

Comments

@getdave
Copy link
Contributor

getdave commented Jan 9, 2020

This Issue is based on the comment below from @youknowriad:

I wonder if we should get rid of that component entirely and just reimplement it in LinkControlSearch 🤷‍♂

Originally posted by @youknowriad in #19458

The dependency on the underlying URLInput is causing an increasing amount of technical debt.

Would should consider a refactor that leaves LinkControl with its own implementation of the functionality in URLInput. This might involve making a new lowerlevel, standalone component that provides the combobox UI which is in turn consumed and controlled by a subcomponent of LinkControl.

@aduth
Copy link
Member

aduth commented Jan 21, 2020

Related: #19677 (review)

I've been thinking about this with regards to some of the ongoing iterations to the LinkControl component. I think there might be some nice outcome where both components continue to exist and each serve a distinct and well-defined role.

The way I view it is:

Original Annotated
original annotated

URLInput: Is a text input field, which presents suggestions when the user enters an input.

  • It renders suggestions, ideally using some generalized UI component for this sort of ComboBox rendering (see Components: Add a WAI-ARIA compliant Combobox. #19657)
  • It handles search fetching, using fetchLinkSuggestions context (maybe prop), just as it does today. The end result is that a consumer would expect to receive a value representing a URL (optionally with associated properties describing this resource like exist today from the search endpoint, which may be but does not necessarily represent a post).
  • It manages arrow keys to navigate to select the presented suggestions
  • It does not: Render any labels or associated chrome
  • It is not: Opinionated about how the URL is applied as a hyperlink (HTML anchor tag)

LinkControl: Composes URLInput and adds labels, maybe reasonable that it maintains "modes" (view/edit) as it does today

  • It is more like what we tend to consider in our "FooControl" abstractions, and is akin to something like TextControl, where it renders a URLInput (analogous to a TextControl's <input type="text">)
  • It can render additional settings relevant for how the URL chosen via URLInput is intended to be presented as a hyperlink

As to how this would be accomplished:

  • Today, URLInput renders as a BaseControl, which we wouldn't want in this updated model of abstraction. It would appear that there is no default label, which would seem to make it feasible to change the default behavior without much difficulty. For existing implementations which would provide a label, it might be a matter of handling this as a deprecation, where the label is passed through to an equivalent rendering of a LinkControl.
  • The URLInput autoFocus prop seems to operate on assumptions that it would be shown in modal contexts. In this updated model of abstraction, we wouldn't want to assume this. I think it would be fair to simply remove this default behavior, but still allow autoFocus to be passed optionally.
  • The URLInput onChange prop callback should be documented to receive a search result as its second argument. I think this is a matter of faulty documentation, because it already does receive the search result, but it's (mistakenly) documented as being a post object.

Open questions:

  • What purpose do we see for the existing URLPopover component? Can it simply be a rendering of a <Popover><LinkControl /></Popover>? It does not hold much value vs. simply rendering Popover directly, but at least this would prevent it being an issue for backward compatibility.
  • How do we represent the value of LinkControl ? Today, it seems to be treated as a combination of the search result and link settings (where link settings can be options like "Opens in New Tab", or custom settings provided through its settings configuration prop). This may be okay, though it could cause some confusion on what valid properties it should be expected to contain.

@aduth
Copy link
Member

aduth commented Jan 21, 2020

The current implementation of LinkControl also customizes how suggestions are rendered by URLInput. I think ideally this is something where this instead becomes the new default rendering of URLInput suggestions, and we don't need to (or want to, for sake of consistent UX) expose any additional options for customizing those suggestions.

@aduth
Copy link
Member

aduth commented Jan 21, 2020

I only just now read through #18061. Sorry if you'd shared it previously and I'd neglected to give it a more thorough read.

I see quite a lot of overlap in our overviews, and I very much agree with your assessment there:

Based on the evidence above, it appears that what is required is not extensive modification or refactoring of the existing URL* components, but rather the creation of a single component which wires together the lower-level components to provide a unified link interface.

Do you see the existence of this issue being contradictory with #18061 ? If this one is proposing to effectively remove URLInput, whereas #18061 is strategizing how best to go about composing with URLInput?

@aduth
Copy link
Member

aduth commented Jan 30, 2020

My understanding based on continued discussion in #18061 is that we won't need to deprecate this component altogether, but that we can use LinkControl as an abstraction to serve in place for much of what URLInput tries to achieve today, and pair down URLInput to a more basic set of features.

As such, let's close this for now, unless we decide to revisit that assessment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Technical Feedback Needs testing from a developer perspective. [Package] Block editor /packages/block-editor
Projects
None yet
Development

No branches or pull requests

2 participants