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

Check possible identifier values for a valid one to use for {{slug}} #1757

Closed
wants to merge 2 commits into from

Conversation

markphilpot
Copy link

Fixes #1700

Summary

This change checks the entry values for IDENTIFIER_FIELDS for one that is valid (truthy) rather than using the first one found (title). I'm not sure this is how the community wants to solve this issue, but this works for my workflow.

Was able to successfully create a post where path was specified and title was left empty (""). For reference, my slug field in my config.yml file was "{{year}}-{{month}}-{{day}}-{{hour}}_{{minute}}_{{second}}" so it didn't contain {{slug}} to begin with.

@markphilpot markphilpot changed the title Check possible identifier values for valid one to use for {{slug}} Check possible identifier values for a valid one to use for {{slug}} Sep 18, 2018
@verythorough
Copy link
Contributor

Deploy preview for netlify-cms-www ready!

Built with commit 8ebf010

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

@verythorough
Copy link
Contributor

Deploy preview for cms-demo ready!

Built with commit 8ebf010

https://deploy-preview-1757--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.

Thanks for the PR!

One issue here is that, with this change, we no longer infer the identifier in a controlled manner. Eg., whichever identifier candidate happens to be first in your config.yml is the one that's used, rather than always using the same order of potential inferred fields. Details in the review.

const fieldNames = collection.get('fields').map(field => field.get('name'));
return IDENTIFIER_FIELDS.find(id => fieldNames.find(name => name.toLowerCase().trim() === id));
Copy link
Contributor

Choose a reason for hiding this comment

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

The only change needed here is doing a filter instead of a find, and returning the array of field names that occurs in both. We could greatly simplify this by using lodash.intersection. This allows the order of IDENTIFIER_FIELDS to dictate which field is used, otherwise this would be a breaking change.

@erquhart
Copy link
Contributor

erquhart commented Nov 2, 2018

#1543 provides the fix needed here.

@erquhart erquhart closed this Nov 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants