Chore - extract link container from rich text#10495
Conversation
…component. Formalise class names.
…Settings callback is provided
| IconButton, | ||
| } from '@wordpress/components'; | ||
|
|
||
| class URLPopover extends Component { |
There was a problem hiding this comment.
I had a couple of quandries about this new component.
- Should it be in the components package instead of editor? It does feel quite guten-specific, so I went with Editor.
- Technically, this could be used to edit anything. As such, I could go as far to call it something like InlineEditor, EditPopover, InlineEditPopover. Possibly worth waiting for that pattern to emerge before naming it something so generic. 🤷♂️
There was a problem hiding this comment.
I think you made the right call!
523ad00 to
49387e6
Compare
noisysocks
left a comment
There was a problem hiding this comment.
This is looking 💯! Left some small comments. Testing this locally now.
There was a problem hiding this comment.
I'm not seeing anything in this example code that calls openURLPopover. Should we add a <button> that demonstrates it? Or just remove it?
There was a problem hiding this comment.
Missing docs for the isEditing prop.
There was a problem hiding this comment.
Now removed the isEditingProp entirely.
There was a problem hiding this comment.
Could make this clearer by referencing isEditing by name.
Callback used to return the React elements that are rendered when `isEditing` is `true`.
There was a problem hiding this comment.
Could make this clearer by referencing isEditing by name.
Callback used to return the React elements that are rendered when `isEditing` is `false`.
There was a problem hiding this comment.
Could make this a little clearer.
Callback used to return the React elements that are rendered inside the `URLPopover` settings drawer that the user can expand open.
| IconButton, | ||
| } from '@wordpress/components'; | ||
|
|
||
| class URLPopover extends Component { |
There was a problem hiding this comment.
I think you made the right call!
There was a problem hiding this comment.
I'm a bad person and this is a dumb nit but I think showSettings && ( <div /> ) looks nicer than using a ternary when in JSX.
| onClickOutside={ onClickOutside } | ||
| > | ||
| <div className="editor-url-popover__row"> | ||
| { isEditing ? renderEditingState() : renderViewingState() } |
There was a problem hiding this comment.
Any thoughts on having isEditing be entirely the responsibility of the parent component? That is, making this line:
{ renderLinkInterface() }There was a problem hiding this comment.
That's a good idea and solves a problem. In the media placeholder there won't be a case where isEditing is false, so something like renderLinkInterface allows that to be expressed in a simpler way.
There was a problem hiding this comment.
Could even use the children prop for a real 🔥 looking API:
<URLPopover
renderSettings={ () => (
<div></div>
) }
>
{ () => (
<div></div>
) }
</URLPopover>
``* Extract rendering logic for LinkContainer into a new presentational component * Give PositionAtSelection its own classname to uncouple it from LinkContainer * Extract link container styles from rich-text into new link container component. Formalise class names. * Add tests for new link container component * Add docs * Provide sane defaults for LinkContainer position and avoid spreading props * Rename component to URLPopover * Export the new component from the editor package * Optionally render the settings button dependent on whether the renderSettings callback is provided * Fix incorrect prop name in docs * Update classnames used in e2e tests * Move responsibility for managing the isEditing state completely to the implementor * Improve docs * Use boolean logic over a ternary * Improve example * Use children instead of RenderURLEditor callback * Add docs for focusOnMount prop
Description
Extracts the presentational aspects of the RichText's LinkContainer component into a new presentational URLPopover component.
The goal is to use this component to render the url editor for inserting media from a url in the MediaPlaceholder. That can be seen on this comment - #9264 (comment). Currently that PR completely reimplements the LinkContainer, and it'd be good to avoid that duplication.
The new component doesn't implement much link editing functionality, it's mostly just a way to reuse the visual side of the popover. Why? The RichText integration was previously tightly coupled to this component, and extracting it in a generic way is something that I don't think is possible.
Also, the URLInput component used in the previous implementation has always-on autocompletion for posts/pages, so is not usable for the MediaPlaceholder. That could also do with refactoring another time.
How has this been tested?
Screenshots
There should be no difference in the UI/UX
Before (on master)
After (this branch)
Types of changes
Refactor (non-breaking change that tidies up code)
Checklist: