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

Update displayed permalink when slug is cleared #11783

Merged
merged 9 commits into from Nov 20, 2018

Conversation

Projects
None yet
4 participants
@earnjam
Contributor

earnjam commented Nov 12, 2018

Description

Closes #7002.

This does a simple fix of the issue in #7002 where a permalink slug appears to stay the same after it is cleared, when it should be generated based off of what is in the title.

With this fix, if the post has not been published yet and the slug has not been manually customized, then the slug continually updates to reflect what is in the post title. Once it's published or the slug is manually edited, it will be frozen to that value. If cleared, it will go back to auto-rendering based off the post title. This is technically the same save functionality as before, it's just displaying a closer representation to the actual value that will be saved.

I also improved on the existing slug cleaning a bit, without going all the way to a full recreation of sanitize_title() in JS. (I also didn't want to call the utility function sanitizeTitle for that reason. If someone has a better name suggestion, please let me know.)

How has this been tested?

  1. Create a new post
  2. Give it a title
  3. Click save draft
  4. Edit the slug
  5. Change the title
  6. Notice remains the custom slug from step 4
  7. Clear the slug and save
  8. Notice it auto-generates the slug from the new post title

Screenshots

permalink-edit

Additional Notes

Even with this change, we still have some work to do to achieve feature parity between the old and new editor when it comes to editing the permalink. As it stands, it can't be edited until a manual save has occurred, which is confusing. Ideally it would become editable on blur from the title, as it was before. However, in order to do that, we need it to be able treat it like a preview link for autosaves and drafts, since the actual permalink URL won't be available yet.

I think the ideal fix here is to abstract out the preview link functionality from the preview button component and allow it to be reused here, but that might need to just be a future enhancement post 5.0. This will work in the short-term to improve the permalink editor.

Fixes #7002

@earnjam earnjam added this to the WordPress 5.0 RC milestone Nov 12, 2018

@earnjam

This comment has been minimized.

Contributor

earnjam commented Nov 12, 2018

Hmm, autosave only saves title, content and excerpt, so the change to generated_slug is causing it to appear dirty and fail the e2e test.

@earnjam

This comment has been minimized.

Contributor

earnjam commented Nov 13, 2018

I'm going to take a different approach on this that doesn't manipulate generated_slug. I'll push something up later today or tomorrow.

@earnjam earnjam force-pushed the fix/update-permalink-on-clear branch from 5451d29 to 0c52144 Nov 13, 2018

@earnjam

This comment has been minimized.

Contributor

earnjam commented Nov 14, 2018

I refactored this to stop editing generated_slug, but functionality is the same. All tests are passing now. It's ready for a review.

@mtias mtias modified the milestones: WordPress 5.0 RC, 4.5 Nov 16, 2018

@earnjam

This comment has been minimized.

Contributor

earnjam commented Nov 17, 2018

I just opened #12009, which goes a good bit further than this one on improvements.

Let's evaluate both and discuss plans for the future of the title permalink component. That will dictate how we handle reusing some of the logic in the new sidebar permalink panel.

*
* @return {string} Processed string
*/
export function cleanForSlug( string ) {

This comment has been minimized.

@youknowriad

youknowriad Nov 19, 2018

Contributor

Seems like a good candidate for a unit test

Show resolved Hide resolved packages/editor/src/components/post-title/index.js Outdated
Show resolved Hide resolved packages/editor/src/components/post-permalink/index.js Outdated
@earnjam

This comment has been minimized.

Contributor

earnjam commented Nov 19, 2018

Thanks @youknowriad. Suggestions look good and I can make those changes. I'm curious whether #12009 is realistic or if you'd rather just ship a fix for this and continue to iterate on something like that.

The same clearing slug problem exists in the sidebar panel, so we should probably fix it there at the same time as this because otherwise it will be pretty confusing. Want me to add that to this PR?

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 19, 2018

The same clearing slug problem exists in the sidebar panel, so we should probably fix it there at the same time as this because otherwise it will be pretty confusing. Want me to add that to this PR?

Yes please, that would be ace

I'm curious whether #12009 is realistic or if you'd rather just ship a fix for this and continue to iterate on something like that.

I'm not certain I understand this PR properly yet to give a proper answer and also I don't feel like I'm the right person for it as it couches some existing core PHP logic.

@earnjam earnjam force-pushed the fix/update-permalink-on-clear branch from 0c52144 to 673a2fa Nov 19, 2018

@earnjam

This comment has been minimized.

Contributor

earnjam commented Nov 20, 2018

I think this is ready for a review.

  • Added a simple unit test for the new cleanForSlug function.
  • Added support for clearing slug on the sidebar.
  • Made it auto-generate the slug/permalink based on the cleaned version of the title as you're typing it in (see gif below)
  • Also cleans up the customized sidebar slug on blur to mimic what it will actually be set as on save.
  • Both title and sidebar now fall back to displaying the post ID as the slug portion of the permalink when there is no slug or title entered (that's what the permalink will use in that case)

sidebar-permalink-editor

@youknowriad

Works great on my testing. Thanks

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Nov 20, 2018

As a follow-up, it might be a good idea to add an e2e test.

@youknowriad youknowriad merged commit f33498b into master Nov 20, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mtias mtias deleted the fix/update-permalink-on-clear branch Nov 20, 2018

grey-rsi pushed a commit to OnTheGoSystems/gutenberg that referenced this pull request Nov 22, 2018

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