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: allow special characters to pass through slugification #6220

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

mikestopcontinues
Copy link

@mikestopcontinues mikestopcontinues commented Feb 17, 2022

Summary

Some of us would like to include / in our slug fields to build nested page structures. This PR allows arbitrary chars to pass through slugification, above and beyond what's allowed by slug.encoding. Useful for / and perhaps for other functionality in future.

Fixes #4963

Test plan

I added new tests for the new parameter I added demonstrating the true and false case. I also updated the dev config.yml with two example collections.

Checklist

Please add a x inside each checkbox:

  • I have read the contribution guidelines.
  • Code is formatted via running yarn format.
  • Tests are passing via running yarn test.
  • The status checks are successful (continuous integration). Those can be seen below.

A picture of a cute animal (not mandatory but encouraged)

Sloth pic

@erezrokah erezrokah added the type: feature code contributing to the implementation of a feature and/or user facing functionality label Feb 17, 2022
@erezrokah erezrokah marked this pull request as ready for review February 18, 2022 14:09
@erezrokah erezrokah requested a review from a team February 18, 2022 14:09
@mikestopcontinues
Copy link
Author

@erezrokah Hey, is this solution acceptable? Should I go ahead and update the tests?

@erezrokah
Copy link
Contributor

Sorry @mikestopcontinues, haven't been able to dig into this.
From a quick look at the issue, the problem seems to be scoped to nested collections and preview_path slugification.
It seems that the expected behavior for nested collections is not to slugify the value of {{slug}}, so maybe we don't need an option and only change that behavior for nested collections?

@mikestopcontinues
Copy link
Author

Hi @erezrokah thanks. I was waaay off!

I updated the PR to specifically target preview paths, under two conditions:

  1. collection.has('nested')
  2. collection.get('preview_preserve_slash')

(2) is because my use-case is a slug field that supports slashes. Nested collections are cool, but they're really hard to use. First is the GUI or lack thereof, but I also query a separate github repo for content, and the nested directories make it very difficult to do efficiently. If you push the issue, I'll remove preview_preserve_slash as I can hack my way forward via (1), though it ends up making things more buggy for my content co-founder.

You can see both the nested-method and the slug-method at work in the dev config.yml.

Two questions:

  1. I strip the first and last slash here, as that seems to be the right call. Is it?
  2. When preserving slashes, I just skip sanitizeFilename. Seemingly all of the characters it sanitize are sanitized already anyway. I would have used their {sanitize: (char) => replace} interface, except they don't pass what it would have decided, so I would have had to completely reimplement the library just to ignore slashes. Didn't seem worth it.

@erezrokah
Copy link
Contributor

Hi @mikestopcontinues, I completely forgot we did #4279 a while back to support the use case described in the issue.

Can you try using {{dirname}} to see if that solves your use case?

@mikestopcontinues
Copy link
Author

Hi @erezrokah

{{dirname}} is a blank field for me. My setup looks like this:

    slug: {{fields.slug}}
    preview_path: /{{fields.slug}}

And fields.slug is just a regex pattern that matches this-is/a-slug.

@mikestopcontinues
Copy link
Author

@erezrokah Sorry to ping you on this, but it's a blocker for my current site upgrade. Any thoughts?

@erezrokah
Copy link
Contributor

erezrokah commented Mar 14, 2022

@erezrokah Sorry to ping you on this, but it's a blocker for my current site upgrade. Any thoughts?

Sorry for the delay @mikestopcontinues. I might not be able to look at this for another week.
I would like to separate the use cases:

  1. preview_path for nested collections - this should be handled by using {{dirname}} - I'll need to verify it works.
  2. preview_path for non nested collections. The change here is more risky as it can impact all users.

If this is blocking you should be able to use the generated CMS build from this Deploy Preview, https://deploy-preview-6220--cms-demo.netlify.app/dist/netlify-cms.js

@mikestopcontinues
Copy link
Author

@erezrokah Thanks! For the time being, I'll try the deploy preview. Let me know what I can do to officially move this forward when you can.

@stale
Copy link

stale bot commented Apr 26, 2023

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.

@martinjagodic
Copy link
Member

@mikestopcontinues are you still interested in moving this forward?

@mikestopcontinues
Copy link
Author

Sorry @martinjagodic, I no longer use Decap.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature code contributing to the implementation of a feature and/or user facing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

preview_path not does not correspond to the real path
3 participants