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

slug fix #1012

Closed
wants to merge 4 commits into from
Closed

slug fix #1012

wants to merge 4 commits into from

Conversation

vencax
Copy link
Contributor

@vencax vencax commented Jan 16, 2018

This fix originally fixed issue with unicode chars in slug (#414).
I hope i won't need to fix that anymore :)

@verythorough
Copy link
Contributor

verythorough commented Jan 16, 2018

Deploy preview for netlify-cms-www ready!

Built with commit 081ef6b

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

@verythorough
Copy link
Contributor

verythorough commented Jan 16, 2018

Deploy preview for cms-demo ready!

Built with commit 081ef6b

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

@vencax
Copy link
Contributor Author

vencax commented Jan 16, 2018

6518df7 is subject for discussion but IMHO files, urls should NOT contain unicode chars.

@tech4him1
Copy link
Contributor

tech4him1 commented Jan 17, 2018

@vencax Can you explain your reasoning as to why Unicode characters should not be allowed by the CMS? It seems trivial to do the processing in your SSG instead. Our reasoning for allowing Unicode chars is in #640:

Right now, we only support Latin chars in slugs/filenames (we strip accents from chars if we can, and drop everything else). This isn't very user-friendly, and latest browsers (including IE11) support Unicode, so we might as well allow users to use it.

This PR upgrades the slug formatter to use the new IRI standards (RFC 3987), which support other than latin-1 chars in slugs, instead of the URI one (RFC 3986).

EDIT: As a matter of specification, URIs with Unicode characters (known as IRIs) are valid HTML5. See https://www.w3.org/TR/html52/infrastructure.html#infrastructure-urls and it's linked articles.

@tech4him1
Copy link
Contributor

tech4him1 commented Feb 16, 2018

ping @vencax. Can you update us on why you think Unicode chars need to be stripped out of URLs/filenames? I think it's just as easy to process URL's in your site generator instead of the CMS, but if you think otherwise, please let us know!

@vencax
Copy link
Contributor Author

vencax commented Feb 16, 2018

As an site admin I want nice urls that does not contain any special chars. I want as well nice filenames in repository -> in media URLs. I use bare GH pages. I use them with NetlifyCMS and I want it to allow BFUs to manage the site without any knowledge of entire sitegenerating process.
This PR is ensuring that no BFU will polute URLs and filenames with any diacritics or any other unicode trash.
But I have to admit, that this can be customisable in settings when other admin does not share my opinions.

@tech4him1
Copy link
Contributor

tech4him1 commented Feb 17, 2018

Exactly, this has to be a matter of opinion. I don't have any problem discussing this more with the other maintainers. Personally, I don't have any problem with this being an option, I just think it's more practical to have static site generators do it. You have outlined a case where it isn't as practical. I think the easiest way would be to allow users to add their own slugification plugins/functions to the CMS, instead of us creating a single default for everyone (either with or without diacritics). Thoughts?

src/lib/slug.js Outdated
@@ -0,0 +1,10 @@
import slug from 'slug';
Copy link
Contributor

Choose a reason for hiding this comment

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

@vencax Just as a note here, one of the main reasons we removed the slug package before was that it was almost doubling the CMS bundle size because it included an entire Unicode lookup table. I think we would need to use a different package here, there were some options discussed in #607.

Copy link
Contributor

Choose a reason for hiding this comment

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

Of course, if it was a plugin, you could use slug if you wanted, no problem there!

@tech4him1
Copy link
Contributor

cc/ @erquhart @Benaiah @talves

Copy link
Collaborator

@talves talves left a comment

Choose a reason for hiding this comment

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

A check could easily be set here for this.implementation.slugFormatter existing on the backend and use it instead. This would allow for a customized slug method. Although it assumes it would have to be a registered backend which is where we are heading soon anyway.

@vencax
Copy link
Contributor Author

vencax commented Feb 17, 2018

What do you mean with registered backend?

@erquhart
Copy link
Contributor

@vencax he means you can register a custom backend via registerBackend, but there are still some details to tie up before you could, say, extend the GitHub backend with your functionality and register that.

The serializeWidgetValues API is an undocumented config options specifically for handling complex values in an intermediary format during editing for performance. It was created for the markdown widget, but currently isn't in use. I know it technically works for this case, but we wouldn't want to use it for that.

If you're not using a static site generator, do you at least have a script where you can strip characters out of filenames at build time?

@vencax
Copy link
Contributor Author

vencax commented Feb 23, 2018

I have only git repo and nothing else. When something change in the repo (gh-pages branch), gh pages jekyll is rebuild. Principle of GH pages ...
When you don't want to use the API, just remove it. Otherwise make it official by documentation. I have this working, so use it, or leave it :)

@tech4him1 tech4him1 changed the title slug fix Strip Unicode from entry filenames. Feb 23, 2018
@tech4him1 tech4him1 changed the title Strip Unicode from entry filenames. Strip Unicode from entry filenames Feb 23, 2018
@tech4him1
Copy link
Contributor

tech4him1 commented Feb 24, 2018

@vencax I've created a new PR to add this as an actual config option to the CMS, can you please review and make sure it works for you? Please let me know if you have any problems with my approach.

PR #1135

@tech4him1 tech4him1 changed the title Strip Unicode from entry filenames slug fix Feb 24, 2018
@erquhart
Copy link
Contributor

Closing in favor of #1135, should be merged shortly.

@erquhart erquhart closed this Mar 19, 2018
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.

5 participants