Conversation
| } | ||
|
|
||
|
|
||
| // joen make me pretty |
There was a problem hiding this comment.
@jasmussen you shouldn't have showed me this tip :)
There was a problem hiding this comment.
Haha love it :D — I'll take a look.
|
Nice work! I'm curious on Ella's thoughts here, as the link feature of the current editor is stellar in how it works. Whatever we could copy from that needs to come along for the ride. That includes the "select text and paste link" feature. There's also a bunch of little enhancements like the link dialog showing in context of the selected text, and easily dismissing with a click outside, as well as an easy way to unlink the link again. Here's a mockup:
To put it differently — how can we port as much as possible of the current link behavior? Is that out of scope for this PR? |
|
Here's a quick mockup of what I was thinking:
|
aduth
left a comment
There was a problem hiding this comment.
Empty file editor/components/format-toolbar/index.js should be removed.
| }, {} ); | ||
|
|
||
| // Link format | ||
| const link = parents.find( parent => parent.nodeName === 'A' ); |
There was a problem hiding this comment.
We're not polyfilling Array#find, this will error in IE11.
| import IconButton from 'components/icon-button'; | ||
|
|
||
| function Toolbar( { controls } ) { | ||
| function Toolbar( { attributes, setAttributes, controls } ) { |
There was a problem hiding this comment.
My first reaction to these changes is that the line is becoming blurred between the concept of a general-purpose toolbar and a block-specific toolbar. We should decide if there's value in the former and whether we can or care to preserve this distinction.
There was a problem hiding this comment.
Well attributes and setAttributes here don't mean "block attributes" and "update block attributes" but I see it as "toolbarState" "updateToolbarState" which is generic enough. Could be renamed but this would impact the attributes setAttributes used in the isActive and onClick of the block controls.
There was a problem hiding this comment.
Well
attributesandsetAttributeshere don't mean "block attributes" and "update block attributes" but I see it as "toolbarState" "updateToolbarState" which is generic enough.
Given that I had trouble making this distinction, I expect others will as well.
| control.onClick( attributes, setAttributes ); | ||
| } } | ||
| className={ classNames( 'editor-toolbar__control', { | ||
| 'is-active': control.isActive && control.isActive( attributes ) |
There was a problem hiding this comment.
This will need to be rebased to account for changes in #500.
| @@ -0,0 +1,105 @@ | |||
| /** | |||
There was a problem hiding this comment.
Should this file have been named link-control.js?
There was a problem hiding this comment.
Oh, I see now the LinkControl component is inlined in the file, but the default export is the array of formatting controls. Hmm...
There was a problem hiding this comment.
Could be two files
| <div className="editable-visual-editor__link-modal"> | ||
| <form onSubmit={ this.submitLinkModal }> | ||
| <input type="url" value={ this.state.value } onChange={ this.updateLinkValue } /> | ||
| <button>{ wp.i18n.__( 'Add Link' ) }</button> |
There was a problem hiding this comment.
Trying to decide if there's value in the <Button /> component (editor/components/button) being the preferred default usage so we can apply styles broadly to all buttons in the editor.
There was a problem hiding this comment.
Yes, this needs to be updated to match @jasmussen's comment
| } ) } | ||
| /> | ||
| { this.state.opened && | ||
| <div className="editable-visual-editor__link-modal"> |
There was a problem hiding this comment.
Can we apply this class to the form and drop the wrapper element?
|
@jasmussen My question about your mockups is when do we show/hide the link "modal". Do we show it automatically when the link is focused? |
I was thinking the behavior should be as 1:1 identical to the current wp-admin behavior as we can make it, to the point that if we could copy the whole thing over (I know that we can't) it would be great.
|
|
closing in favor of #524 |
This PR adds the link control to the formatting toolbar.
I had to change how the
Toolbarworks to allow advanced controls showing Modals and updating the state accordingly.Testing instructions