Skip to content
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

Show updated permalink on status bar. #24380

Closed

Conversation

yansern
Copy link
Contributor

@yansern yansern commented Aug 5, 2020

Description

When hovering over the permalink in the document sidebar on the block editor, the status bar should show the updated permalink.

When clicking on the permalink, it should still be able to open the post in a new window (by using the postLink) even though the permalink hasn't been created yet.

See Automattic/wp-calypso#41948 for more details.

How has this been tested?

Tested this in my local wp-env with permalink enabled on settings page.

Screenshots

image

image

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.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR.

@@ -104,8 +107,18 @@ function PostLink( {
<div className="edit-post-post-link__preview-link-container">
<ExternalLink
className="edit-post-post-link__link"
href={ postLink }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

postlink is coming from the getCurrentPost selector's link property, which in turn comes from the REST API (see). That field is set by the corresponding controller to use WordPress's get_permalink() function.

It might be worth considering to modify that function, in order to change the behavior for all cases where the permalink is used (including wp-admin Posts screen and Classic Editor), in order to get perfectly consistent behavior everywhere 🤔

Comment on lines +112 to +121
onClick={ ( event ) => {
// This shows the permalink in the status bar,
// while uses the postLink to open in a new window.
event.preventDefault();
window.open(
postLink,
'_blank',
'noreferrer,noopener'
);
} }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather not hack the <a href="" />, since it makes for some really unexpected behavior. As others have elaborated in #41948, the permalink should actually work for drafts as well.

Suggested change
onClick={ ( event ) => {
// This shows the permalink in the status bar,
// while uses the postLink to open in a new window.
event.preventDefault();
window.open(
postLink,
'_blank',
'noreferrer,noopener'
);
} }

@@ -104,8 +107,18 @@ function PostLink( {
<div className="edit-post-post-link__preview-link-container">
<ExternalLink
className="edit-post-post-link__link"
href={ postLink }
href={ isEditable ? permalink : postLink }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do this, we might want to use addQueryArg to add the preview bool to permalink (in case the post is still in draft state).

Copy link
Contributor

@ockham ockham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is looking good to me overall, but I'd rather not add the onclick handler.

Since this changes the draft preview behavior quite a bit, I'd like to defer other folks for final approval.

cc/ @youknowriad @mcsf

@Copons
Copy link
Contributor

Copons commented Aug 24, 2020

I'm not super into hacking the href as well, especially for such a minor feature.

What about adding an info text that shows up when the post doesn't have a permalink yet, explaining that the permalink only works once the post is published? 🤔

Other options:

  • Remove the whole "view post" from the Permalink section. The post can be (pre-)viewed with the Preview button in the header, and I'd argue that the Permalink section is a poor location for a View Post link.
  • Show the full permalink, but don't link it. Although this might be awkward if folks copy the permalink and try to access it directly. (Same could be said with the linked permalink, for that matter).

@david-szabo97
Copy link
Member

david-szabo97 commented Sep 2, 2020

I'm not super into hacking the href as well, especially for such a minor feature.

👍

I went through the whole flow just to see what's going on:

  1. Create Post - View Post link is showing a 404 - Href and the link text is equal
  2. Add post title and save as draft - View Post link opens ?p= URL - Href is set to ?p=, link text shows the slug, opening link text is showing a 404
  3. Scheduled post - View Post link opens ?p= URL - Href is set to ?p=, link text shows the slug, opening the link text is showing the post (if I'm logged in, otherwise it's a 404)
  4. Published post - View Post link opens ?p= URL - Href is set to the slug, link text shows the slug, opening the link text is showing the post

In my opinion, once the post is scheduled, the link text should point to the post link (one with the slug).
I do understand the problem from a marketer's viewpoint, it would be great to somehow fix this minor issue. It's confusing to see the link but being redirected to a different one. 😕

What about adding an info text that shows up when the post doesn't have a permalink yet, explaining that the permalink only works once the post is published? 🤔

I think this would be a great approach. I'd prefer the link to always point to the permalink and have a text to explain why it's not working yet.

@mcsf
Copy link
Contributor

mcsf commented Sep 10, 2020

There is some prior discussion on permalinks here: #21410 (comment). I recommend reading it, as some compromises were made there that are useful to this PR.

Essentially, one of the important things to look out for is what kind of guarantees WordPress can make about permalinks, and doing our best not to break that. Looking at what the Classic editor does can also often be useful in this domain.

In my opinion, once the post is scheduled, the link text should point to the post link (one with the slug).

I tend to agree with this.

One thing I'm not a fan of, though, is crafting ad hoc logic for generating permalinks. This puts us at risk of introducing subtle bugs, maintenance difficulties, and in general falling out of sync with the true decision maker (the WP back end):

// Using array join prevents undefined/null values
permalink = [ permalinkPrefix, postSlug, permalinkSuffix ].join( '' );

This issue is also present to an extent in #21410, but becomes more apparent here. It's also a direct duplication of what happens inside the getPermalink selector. So, in short:

  • The WP API allows us to access the source of truth that is the WP backend
  • The @wordpress/data is the "official" data layer mediating access to WP API
  • The data layer also implements some local business logic, as is the case in getPermalink
  • Thus, the best guarantees that the editor can make about user data (e.g. a future post's permalink) come when we feed off the data layer exclusively

So it's important for this PR to refactor so as to use selector data as directly as possible, even if that requires refactoring selectors.

@yansern
Copy link
Contributor Author

yansern commented Sep 28, 2020

As the issue is fairly trivial, I have no mental capacity at this point to look into this. Feel free to supercede this PR!

Base automatically changed from master to trunk March 1, 2021 15:43
@ramonjd ramonjd added [Type] Enhancement A suggestion for improvement. [Package] Edit Post /packages/edit-post labels Aug 26, 2021
@youknowriad
Copy link
Contributor

Going to close this PR for now. Thanks for your efforts.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Package] Edit Post /packages/edit-post [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants