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

Try: Improve permalink editing #12009

Merged
merged 9 commits into from
Mar 18, 2020
Merged

Try: Improve permalink editing #12009

merged 9 commits into from
Mar 18, 2020

Conversation

earnjam
Copy link
Contributor

@earnjam earnjam commented Nov 17, 2018

Description

Allows immediate editing of permalinks, without requiring a save.
Fixes: #7129, #12714, #12031

Notes:

  • In order edit the permalink before a save, we need to know what the permalink template structure is on page load. To get this, we need to modify get_sample_permalink() in core to add the auto-save status to the list of statuses that are faked to publish. See: /wp-admin/includes/post.php#L1310. In the PR, I just used the get_sample_permalink filter at the end of the function and passed the parameters back through it, pretending it was published. I will need to open a separate core ticket to address this for whenever it's merged.

How has this been tested?

Extensive testing across various post statuses and different orders of editing the slug and/or title and the different methods of saving, publishing and transitioning the post status.

@earnjam earnjam added the [Feature] Permalink The permalink of a post or page and the experience of setting or editing it label Nov 17, 2018
@afercia
Copy link
Contributor

afercia commented Nov 18, 2018

Related: #12031

@earnjam earnjam force-pushed the try/improve-permalink-editing branch from 73a5d50 to 038395d Compare November 20, 2018 20:29
@earnjam
Copy link
Contributor Author

earnjam commented Nov 20, 2018

Rebased and resolved merge conflicts. Still need to apply the updates to the sidebar panel too, but not sure the level of refactoring that should be done at this point right before an RC.

Should note again that this PR would require a core patch at merge time.

@noisysocks noisysocks added this to the 4.8 milestone Dec 10, 2018
@youknowriad youknowriad modified the milestones: 4.8, 4.9 Dec 19, 2018
@youknowriad
Copy link
Contributor

youknowriad commented Jan 22, 2019

Thanks @earnjam for this PR and sorry for the delays, I'd like to see #7129 fixed if we can. It seems that this PR does a lot of things in parallel, do you think it's possible to split it into steps or does it only make sense as a single PR?

@youknowriad youknowriad requested a review from a team January 22, 2019 07:56
@earnjam
Copy link
Contributor Author

earnjam commented Jan 22, 2019

@youknowriad I'll look over this again today and see if I can split it up a bit. I think the only thing we could split out would be the application of the preview functionality, though I do think that would be nice to have.

The other question around this, would it be more helpful to have a shared permalink component to use between both the title and the sidebar permalink editor instead of duplicating the effort?

@youknowriad
Copy link
Contributor

The other question around this, would it be more helpful to have a shared permalink component to use between both the title and the sidebar permalink editor instead of duplicating the effort?

IMO, the title permalink should be deprecated at some point, but if it's easy to factorize a shared component, then, yes, it sounds like a great idea.

@earnjam
Copy link
Contributor Author

earnjam commented Feb 15, 2019

I rebased master, stripped out the preview functionality, moved the slug logic to a new getEditedPostSlug selector, and cleaned up the commits to make it easier to follow/read the changes here.

I think this probably the minimum required to fix #7129 to allow permalink editing immediately without requiring any saves.

There are more improvements that I'll probably try to make in separate PRs, such as recreating the logic from the sanitize_permalink() php function to make it a more exact match to what will be saved.

@earnjam earnjam removed the [Status] Stale Gives the original author opportunity to update before closing. Can be reopened as needed. label Mar 13, 2020
Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

The code is looking very good here. Do you think we should land this before the the core patch https://core.trac.wordpress.org/ticket/46266 or maybe wait until the approach is validated. Who do you think can validate that patch?

@earnjam
Copy link
Contributor Author

earnjam commented Mar 16, 2020

@youknowriad I'm going to loop @desrosj in today to get his feedback on the core side and see if we should validate with anyone else, but I don't anticipate it being an issue.

Copy link
Contributor

@youknowriad youknowriad left a comment

Choose a reason for hiding this comment

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

This seems to work well for me. 👍

@youknowriad
Copy link
Contributor

Can we rebase and land this?

This somewhat diminishes the effects of this pull request, but it will
help it pass the e2e tests and also streamlines writing a bit. Reverts
back to preventing showing the permalink editor in the title until after
a manual or autosave. It does at least show the actual permalink on
the autosave and users can edit the slug at any time using the
document sidebar.
@earnjam earnjam force-pushed the try/improve-permalink-editing branch from c4830d3 to ddeeb8d Compare March 18, 2020 12:23
@earnjam
Copy link
Contributor Author

earnjam commented Mar 18, 2020

Rebased. I'll wait for tests to finish and then land it.

@earnjam earnjam merged commit 8ee1f0c into master Mar 18, 2020
@earnjam earnjam deleted the try/improve-permalink-editing branch March 18, 2020 13:20
@github-actions github-actions bot added this to the Gutenberg 7.8 milestone Mar 18, 2020
@youknowriad
Copy link
Contributor

Great work here and thanks for pushing it to the finish line.

@youknowriad
Copy link
Contributor

Should we still close the three issues mentioned here #7129, #12714, #12031

@earnjam
Copy link
Contributor Author

earnjam commented Mar 18, 2020

Yep, all 3 are closed now. The last one had some additional discussion on it, but the original basis of the ticket is resolved. I told Andrea he could open it back up if he felt it wasn't resolved.

@paaljoachim
Copy link
Contributor

paaljoachim commented Mar 24, 2020

I found it interesting that clicking Edit having the save button visible one could click it or just click outside into empty space or another block to automatically close it. Perhaps this method can be used with other features as well. Instead of adding a cancel button one just clicks outside the button and the area it belongs to.
I am thinking about the creation of reusable block and one wants to cancel the creation. #12940

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Permalink The permalink of a post or page and the experience of setting or editing it [Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Permalinks: cannot define post/page permalink until after clicking Save Draft