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

feat(netlify-cms-core) add slug customization support #1931

Closed
wants to merge 7 commits into from

Conversation

barthc
Copy link
Contributor

@barthc barthc commented Dec 4, 2018

Summary
Closes #1164 #445

Picks up where #1239 left off, but instead of throwing an error for duplicate slug, it tries to generate a unique slug by append a number just like the way WordPress does it. For example assuming we already have a slug entry title and a sanitize_replacement option value as - trying to create another entry with slug string title would generate a slug title-1 , title-2 and so on.

To enable slug customization, add a config collection setting slug_field with value pointing to the name of the widget field preferrable a string widget. A couple of things to note regarding the slug field behaviour. On new entry, once the identifier field looses focus, if the slug field is blank, the normal automatic slug string is generated and set as value of the slug field. Also when the slug field looses focus, the input value is sanitized immediately and value updated. The slug field value is not saved in frontmatter.
So if there is a slug change for any published or unpublished entry, a new entry is created using the new slug and the old slug entry is deleted.

Tested changes with the test-repo, github,gitlab and bitbucket backend.

@verythorough
Copy link
Contributor

verythorough commented Dec 4, 2018

Preview proposed changes to netlifycms.org in the link below:

Built with commit 12cfd3d

https://deploy-preview-1931--netlify-cms-www.netlify.com

@verythorough
Copy link
Contributor

verythorough commented Dec 4, 2018

Preview proposed changes to the CMS demo site in the link below:

Built with commit 12cfd3d

https://deploy-preview-1931--cms-demo.netlify.com

Copy link
Contributor

@erquhart erquhart left a comment

Choose a reason for hiding this comment

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

@barthc great work here, thanks for this!

I'd like for us to bring this in as a beta feature, so we can iterate based on feedback without fear of breaking changes. The fact that you made this opt-in via slug_field enables that 👍. Would you mind adding a blurb to the top of the beta features doc on this branch?

If we consider this a developer focused feature that minimally satisfies some critical gaps in the CMS, I can forego much of the product level feedback I would otherwise give. This is a great step in the right direction and does indeed satisfy the issues mentioned in the OP.

The only thing that may be worth addressing before merge (beyond the bits I commented on in the review) is that the identifier (title) field only updates the slug field during the initial focus. So if I enter a title, blur, then edit the title a little more, the edits aren't reflected in the slug. If you're feeling pretty ready to just get this merged, I'm cool with handling that in a subsequent issue.

Sorry this took so long, and thanks again for the killer effort.

if (!state.get('entities')) return null;
return state
.get('entities')
.filter((v, k) => k.includes(collection))
Copy link
Contributor

Choose a reason for hiding this comment

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

This is subject to false positives where the slug happens to include the collection name.

const entryData = entry.get('data');
const unavailableSlugs = boundSelectSlugs(collection.get('name'));
const availableSlugs = [entry.get('slug'), entry.getIn(['metaData', 'parentSlug'])];
const indentifierField = collection && selectIdentifier(collection);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be identifierField.

if (!state.get('entities')) return null;
return state
.get('entities')
.filter((v, k) => k.includes(collection))
Copy link
Contributor

Choose a reason for hiding this comment

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

Subject to false positives.

@barthc barthc force-pushed the 445-customize-sluggification branch from b74ef10 to 61abc02 Compare February 24, 2019 18:44
@barthc
Copy link
Contributor Author

barthc commented Feb 26, 2019

@erquhart Could you please take a look again, pushed some updates.

is that the identifier (title) field only updates the slug field during the initial focus. So if I enter a title, blur, then edit the title a little more, the edits aren't reflected in the slug.

The idea is to only sync the slug field with the identifier_field only if it's empty, incase the user would like to used something different.

@erquhart
Copy link
Contributor

That makes sense if the identifier field is not empty on load. If it's empty on load, though, any edits between load and save should be reflected in the slug.

@barthc
Copy link
Contributor Author

barthc commented Feb 28, 2019

That makes sense if the identifier field is not empty on load. If it's empty on load, though, any edits between load and save should be reflected in the slug.

Yup, I believe that's the behavior of the slug field at the moment. Try creating new entry on the demo.

@barthc
Copy link
Contributor Author

barthc commented Feb 28, 2019

So basically for new entry, if the user has the slug field filled out first before the identifier_field we respect the value no need to sync, or if the user fills out the identifier_field first the slug_field is synced immediately, and then in case the user modifies the slug field, later on, any further changes to identifer_field is not reflected. As I said the idea is to present the user with the auto-generated slug first, the user can choose to go with those or modify. The slug now has it's own field the user should be able to modify as needed without much interference. I just think it is user-friendly this way.

@barthc barthc mentioned this pull request Mar 4, 2019
@stale
Copy link

stale bot commented Oct 29, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@erezrokah
Copy link
Contributor

Closing this as stale, I think we can re-visit in the future as use this work as reference.

@erezrokah erezrokah closed this Apr 1, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Prevent overwriting existing file
4 participants