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

Chrome: Add a Post Publish Panel #4158

Merged
merged 2 commits into from Jan 5, 2018

Conversation

Projects
None yet
6 participants
@youknowriad
Contributor

youknowriad commented Dec 25, 2017

This PR updates the Publish Panel and shows some additional information when the Post is published

screen shot 2017-12-25 at 10 45 09

blocked by #4157 and closes #3496 and closes #3279

@youknowriad youknowriad self-assigned this Dec 25, 2017

@youknowriad youknowriad requested a review from jasmussen Dec 25, 2017

<a href={ post.link }>{ post.title || __( '(no title)' ) }</a>{ __( ' is now live.' ) }
</PanelBody>
<PanelBody>
<div><strong>{ __( 'What\'s next?' ) }</strong></div>

This comment has been minimized.

@mtias

mtias Jan 3, 2018

Contributor

Prefer to use "What's next?" in these cases instead of escaping.

@mtias

mtias Jan 3, 2018

Contributor

Prefer to use "What's next?" in these cases instead of escaping.

This comment has been minimized.

@youknowriad

youknowriad Jan 3, 2018

Contributor

We may have to tweak our ESlint settings to do so.

@youknowriad

youknowriad Jan 3, 2018

Contributor

We may have to tweak our ESlint settings to do so.

This comment has been minimized.

@aduth

aduth Jan 3, 2018

Member

I prefer as currently proposed 😉

@aduth

aduth Jan 3, 2018

Member

I prefer as currently proposed 😉

display: flex;
align-content: space-between;
> * {

This comment has been minimized.

@mtias

mtias Jan 3, 2018

Contributor

Why need this selector? It generally degrades performance a tiny bit.

@mtias

mtias Jan 3, 2018

Contributor

Why need this selector? It generally degrades performance a tiny bit.

This comment has been minimized.

@youknowriad

youknowriad Jan 4, 2018

Contributor

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?

@youknowriad

youknowriad Jan 4, 2018

Contributor

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?

Show outdated Hide outdated editor/components/post-publish-panel/postpublish.js Outdated
} );
clearTimeout( this.dismissCopyConfirmation );
this.dismissCopyConfirmation = setTimeout( () => {

This comment has been minimized.

@aduth

aduth Jan 3, 2018

Member

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.

@aduth

aduth Jan 3, 2018

Member

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.

This comment has been minimized.

@youknowriad

youknowriad Jan 4, 2018

Contributor

mmm! sounds like a good idea to me.

@youknowriad

youknowriad Jan 4, 2018

Contributor

mmm! sounds like a good idea to me.

Show outdated Hide outdated editor/components/post-publish-panel/postpublish.js Outdated
Show outdated Hide outdated editor/components/post-publish-panel/postpublish.js Outdated
Show outdated Hide outdated editor/components/post-publish-panel/postpublish.js Outdated
@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Jan 4, 2018

Contributor

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!

Contributor

jasmussen commented Jan 4, 2018

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

This comment has been minimized.

Show comment
Hide comment
@youknowriad

youknowriad Jan 5, 2018

Contributor

Merging soon if no more concerns here?

Contributor

youknowriad commented Jan 5, 2018

Merging soon if no more concerns here?

@youknowriad youknowriad merged commit 255fe64 into master Jan 5, 2018

3 checks passed

codecov/project 39.31% (-0.13%) compared to daab739
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@youknowriad youknowriad deleted the update/post-publish-flow branch Jan 5, 2018

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Jan 5, 2018

Contributor

The URL readonly field misses an associated label element, noted also in #4187

Contributor

afercia commented Jan 5, 2018

The URL readonly field misses an associated label element, noted also in #4187

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment