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

fix: Show permalink editor in editor #7494

Merged
merged 2 commits into from Jun 26, 2018

Conversation

Projects
None yet
4 participants
@tofumatt
Member

tofumatt commented Jun 22, 2018

Description

Fixed the permalink editor not appearing. Fix #7467.

How has this been tested?

Tested locally and saw the permalink editor appear as it used to in v3.0.1.

Screenshots

2018-06-22 19 29 21

Types of changes

Bug fix.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.

@tofumatt tofumatt requested a review from WordPress/gutenberg-core Jun 22, 2018

@@ -123,7 +123,13 @@ class PostPermalink extends Component {
export default compose( [
withSelect( ( select ) => {
const { isEditedPostNew, isPermalinkEditable, getEditedPostPreviewLink, getPermalink, isCurrentPostPublished } = select( 'core/editor' );
const {
isEditedPostNew,

This comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

I was messing about with possibly improving how the logic to display the permalink editor worked and it meant editing the selectors used… I didn't end up doing it, but I found this a bit easier to read so left it in 😄

@tofumatt tofumatt changed the title from fix: Show permalink editor in editor (fix #7467) to fix: Show permalink editor in editor Jun 22, 2018

@@ -60,7 +60,7 @@ class PostPermalink extends Component {
const { isCopied, isEditingPermalink } = this.state;
const ariaLabel = isCopied ? __( 'Permalink copied' ) : __( 'Copy the permalink' );
if ( isNew || ! previewLink ) {
if ( isNew ) {

This comment has been minimized.

@mcsf

mcsf Jun 22, 2018

Contributor

Can you provide context on why the necessary condition has changed? Put differently: what changed in the breaking PR (#7130) that requires adapting this line?

This comment has been minimized.

@tofumatt

tofumatt Jun 22, 2018

Member

So I'm a bit confused as to how it worked before and why this condition is here at all. The only time we shouldn't render the permalink editor is when the post is entirely new and hasn't been sent to the server (eg isNew === true).

Changing this check makes the Permalink editor behave as it did in 3.0.1… though I'll admit the lack of tests on this had me confused as to exactly what that was, so I just manually tested it 😄

To be honest it's just an educated guess but I'd think https://github.com/WordPress/gutenberg/pull/7130/files#diff-36dbe9dade73983f5ea95f585450ff2cR1091 is the breaking change. Not sure why it's there… @aduth?

This comment has been minimized.

@aduth

aduth Jun 25, 2018

Member

#7130 moved the preview_link out of the posts endpoint into the autosaves endpoint. The autosave state had never previously tracked preview_link, so the referenced line from the pull request is related -- though not directly. Omitting the field is not the breakage; it's the fact that we no longer track preview_link in the objects accessed by getEditedPostPreviewLink. This is intentional, though apparently I did not audit the existing code to update all places where we relied on it.

In fact, the selector getEditedPostPreviewLink should either be removed or updated to getAutosaveAttribute( 'preview_link' ) (as was done for PostPreviewButton).

That said, there's a few issues:

  • We don't populate autosave when the post loads, so we can't rely on it existing to retrieve a preview link. Related: #7416.
  • The behavior in #7130 was changed to be such that the preview button redirects its popup when a preview link becomes available, so there is a short time between an autosave starting and its completion that the preview link is unset. What are we to be rendering as a link here in the meantime?
  • More generally, what is the behavior of the link when there are unsaved changes? It's odd we apply the preview link, but not the mechanics of autosaving in order to show the latest preview of the post.

So I'm inclined to wonder: Should we just set this to be the post.link from whatever is the most recent saved version of the post (currentPost)?

There may be some additional context buried in #5756 which could shed light on why we might want a preview link here.

This comment has been minimized.

@tofumatt

tofumatt Jun 25, 2018

Member

Using getCurrentPost() means we can otherwise preserve the logic here. Thanks! 👍

@@ -60,7 +60,7 @@ class PostPermalink extends Component {
const { isCopied, isEditingPermalink } = this.state;
const ariaLabel = isCopied ? __( 'Permalink copied' ) : __( 'Copy the permalink' );
if ( isNew || ! previewLink ) {
if ( isNew ) {

This comment has been minimized.

@aduth

aduth Jun 25, 2018

Member

#7130 moved the preview_link out of the posts endpoint into the autosaves endpoint. The autosave state had never previously tracked preview_link, so the referenced line from the pull request is related -- though not directly. Omitting the field is not the breakage; it's the fact that we no longer track preview_link in the objects accessed by getEditedPostPreviewLink. This is intentional, though apparently I did not audit the existing code to update all places where we relied on it.

In fact, the selector getEditedPostPreviewLink should either be removed or updated to getAutosaveAttribute( 'preview_link' ) (as was done for PostPreviewButton).

That said, there's a few issues:

  • We don't populate autosave when the post loads, so we can't rely on it existing to retrieve a preview link. Related: #7416.
  • The behavior in #7130 was changed to be such that the preview button redirects its popup when a preview link becomes available, so there is a short time between an autosave starting and its completion that the preview link is unset. What are we to be rendering as a link here in the meantime?
  • More generally, what is the behavior of the link when there are unsaved changes? It's odd we apply the preview link, but not the mechanics of autosaving in order to show the latest preview of the post.

So I'm inclined to wonder: Should we just set this to be the post.link from whatever is the most recent saved version of the post (currentPost)?

There may be some additional context buried in #5756 which could shed light on why we might want a preview link here.

const {
isEditedPostNew,
isPermalinkEditable,
getEditedPostPreviewLink,

This comment has been minimized.

@aduth

aduth Jun 25, 2018

Member

Noted above: getEditedPostPreviewLink is a useless selector, should be removed here (and altogether, since this is its only remaining usage).

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 25, 2018

Thanks for all the context here @aduth; I'll have another look soon and see about using post.link as mentioned.

tofumatt added some commits Jun 22, 2018

@tofumatt tofumatt requested review from mcsf and aduth Jun 25, 2018

@tofumatt

This comment has been minimized.

Member

tofumatt commented Jun 25, 2018

Cool, used getCurrentPost(), tested it locally, and it all worked. r?

@noisysocks

Tested this locally and it smells nice 👍

*
* @return {string} Preview URL.
*/
export function getEditedPostPreviewLink( state ) {

This comment has been minimized.

@noisysocks

noisysocks Jun 25, 2018

Member

Should we deprecate this? On the one hand, it's a public 'method'. On the other, it's undocumented.

This comment has been minimized.

@tofumatt

tofumatt Jun 25, 2018

Member

It's not documented, nothing else uses it, and @aduth mentioned it just missed the cutting block during another commit. I think technically maybe we should but I feel like we're in the clear.

I will add it back, deprecate it, and wear the dunce hat if it affects anyone in the next release 😉

This comment has been minimized.

@aduth

aduth Jun 26, 2018

Member

Worth noting: It doesn't work as-is anyways, hence the original issue. Probably better to not work than to throw errors though. If we wanted full compatibility, we might redirect the result to getAutosaveAttribute( 'preview_link' )

This comment has been minimized.

@tofumatt

tofumatt Jun 26, 2018

Member

But getAutosaveAttribute( 'preview_link' ) won't work on page load, right? I tested this with new page loads of existing posts and "Add New" posts and it never broke the page 🤷‍♂️

@tofumatt tofumatt merged commit 04c0ea2 into master Jun 26, 2018

2 checks passed

codecov/project 46.75% (-0.02%) compared to e5e7335
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt deleted the fix/7467-permalink-editor branch Jun 26, 2018

@tofumatt tofumatt added this to the 3.2 milestone Jun 26, 2018

oxyc added a commit to generoi/gutenberg that referenced this pull request Jun 26, 2018

Merge branch 'master' of https://github.com/WordPress/gutenberg
* 'master' of https://github.com/WordPress/gutenberg: (69 commits)
  fix: Show permalink editor in editor (WordPress#7494)
  Fix text wrapping in Firefox. (WordPress#7472)
  Try another approach to fixing the sibling inserter in Firefox (WordPress#7530)
  fix: Improve "add block" text in NUX onboarding (WordPress#7511)
  Implement core style of including revisions data on Post response (WordPress#7495)
  Testing: Add e2e test for PluginPostStatusInfo (WordPress#7284)
  Add end 2 end test for sidebar behaviours on mobile and desktop. (WordPress#6877)
  Only save metaboxes when it's not an autosave (WordPress#7502)
  Fix broken links in documentation (WordPress#7532)
  Remove post type 'viewable' compatibility shim (WordPress#7496)
  Fix typo. (WordPress#7528)
  Blocks: Remove wrapping div from paragraph block (WordPress#7477)
  fix: change import for InnerBlocks (WordPress#7484)
  Polish library just a teeeeensy bit (WordPress#7522)
  feat: Add snapshot update script (WordPress#7514)
  Display server error message when one exists (WordPress#7434)
  Fix issues with gallery in IE11. (WordPress#7465)
  Polish region focus style (WordPress#7459)
  Fix IE11 formatting toolbar visibility (WordPress#7413)
  Update plugin version to 3.1. (WordPress#7402)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment