Chrome: Add a Post Publish Panel #4158
Conversation
| border-radius: $button-style__radius-roundrect; | ||
| margin: 15px 0; | ||
| background: $white; | ||
| overflow: hidden; |
There was a problem hiding this comment.
Is there a styling issue here? Looks like there's only two spaces instead of a tab - same as next line.
There was a problem hiding this comment.
yep, there's a tabs/spaces mix :)
youknowriad
force-pushed
the
update/post-publish-flow
branch
from
0c97780
to
8b02f7a
Compare
| label={ __( 'Close Publish Panel' ) } | ||
| /> | ||
| </div> | ||
|
|
| <a href={ post.link }>{ post.title || __( '(no title)' ) }</a>{ __( ' is now live.' ) } | ||
| </PanelBody> | ||
| <PanelBody> | ||
| <div><strong>{ __( 'What\'s next?' ) }</strong></div> |
There was a problem hiding this comment.
Prefer to use "What's next?" in these cases instead of escaping.
There was a problem hiding this comment.
We may have to tweak our ESlint settings to do so.
| /> | ||
| <div className="post-publish-panel__postpublish-buttons"> | ||
| <Button className="button" href={ post.link }> | ||
| { __( 'View Post' ) } |
There was a problem hiding this comment.
This label would need to account for "Page" and CPTs.
There was a problem hiding this comment.
This label would need to account for "Page" and CPTs.
Specifically, we could use the type.labels.view_item label from /wp/v2/types?context=edit.
| display: flex; | ||
| align-content: space-between; | ||
|
|
||
| > * { |
There was a problem hiding this comment.
Why need this selector? It generally degrades performance a tiny bit.
There was a problem hiding this comment.
Since the "ClipboardButton" needs a wrapper, we can't target children buttons using a selector. I can replace it with something like > .components__button, > span, what do you think? I like the genericity of > * though?
| */ | ||
| import { getCurrentPost } from '../../store/selectors'; | ||
|
|
||
| class PostPublishPanelPostpublish extends Component { |
There was a problem hiding this comment.
A bit of inconsistency here between the name of the class and the name of the file. I'm not entirely sure if the correct spelling would be "Postpublish" (file name postpublish.js) or "PostPublish" (file name post-publish.js) but the two should align.
There was a problem hiding this comment.
Actually, there's no inconsistency. The PostPublishPanel in the classname refers to the foldeer and the Postpublish to the filename. Hard to distringuish between PostPublish and Postpublish though? Maybe we'd want to change the name for the component entirely?
| } ); | ||
|
|
||
| clearTimeout( this.dismissCopyConfirmation ); | ||
| this.dismissCopyConfirmation = setTimeout( () => { |
There was a problem hiding this comment.
There's no issue here, but this pattern is so easy to get wrong, makes me wonder if we ought to explore a higher-order component like https://www.npmjs.com/package/@hocs/safe-timers or our own.
There was a problem hiding this comment.
mmm! sounds like a good idea to me.
| type="text" | ||
| readOnly | ||
| value={ post.link } | ||
| onClick={ this.onSelectInput } |
There was a problem hiding this comment.
Should this be onFocus for keyboard navigation?
| <div><strong>{ __( 'What\'s next?' ) }</strong></div> | ||
| <input | ||
| className="post-publish-panel__postpublish-link-input" | ||
| type="text" |
There was a problem hiding this comment.
type="text" is default, so this is redundant, unless you think it's wise to be explicit here.
There was a problem hiding this comment.
No strong opinion, just a habit
|
|
||
| export default connect( | ||
| state => ( { | ||
| post: getCurrentPost( state ), |
There was a problem hiding this comment.
Minor: Do we need the entire post object if we're only using post.link in the component?
There was a problem hiding this comment.
We use the "title" as well.
|
Designwise, love it. @karmatosed and I have been discussing some further improvements to this, like showing a checkmark being animated into being when the publishing is complete, but nothing that should be part of this PR, nor anything incompatible with what you've built here. Nice! |
youknowriad
force-pushed
the
update/post-publish-flow
branch
2 times, most recently
from
0a0cb63
to
b076507
Compare
youknowriad
force-pushed
the
update/post-publish-flow
branch
from
55e1798
to
55196f1
Compare
|
Merging soon if no more concerns here? |
|
The URL readonly field misses an associated label element, noted also in #4187 |
This PR updates the Publish Panel and shows some additional information when the Post is published
blocked by #4157 and closes #3496 and closes #3279