Site Editor: Limit template part slugs to Latin chars #38695
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Mamaduka
added
[Feature] Site Editor
Mamaduka
changed the title
|
Size Change: +40 B (0%) Total Size: 1.14 MB
ℹ️ View Unchanged
|
kevin940726
reviewed
Feb 11, 2022
There was a problem hiding this comment.
Are there plans to support non-Latin characters? I'm afraid that this fix could have some side effects when the cleaned slugs collide with each other. For instance hello-世界 and hello-台灣 will both result in the same slug hello-.
Maybe instead of handling them implicitly, we should throw an error and let the users choose the alternative slug names.
Comment on lines
+38
to
+42
| // Currently template parts only allow latin chars. | ||
| // Fallback slug will receive suffix by default. | ||
| const cleanSlug = | ||
| kebabCase( title ).replace( /[^\w-]+/g, '' ) || | ||
| 'wp-custom-part'; |
There was a problem hiding this comment.
Could we extract this into a utility function or hook?
There was a problem hiding this comment.
We have the cleanForSlug method, which supports non-Latin chars; this is the reason I decided to inline it to avoid confusion in the future.
There was a problem hiding this comment.
We could still name this getCleanSlugForLatinChars or something to make it explicit, but I don't feel strongly that we need to do this 👍 .
There was a problem hiding this comment.
I will leave inline for now, if that's okay for you.
I'm not 100% sure about this. Currently, slugs are considered 1:1 map to template filenames, which usually use Latin chars.
WP will try to make them unique and suffix one with a number.
Do would be my ideal UX as well. Now modals on have title fields. Do we always want to display the slug field or just when the title contains non-Latin chars? @jameskoster, do you have any suggestions about displaying the slug field? |
|
Is there any situation in the general UX where the slug has a practical use for template parts? I'm trying to think of something but coming up blank 🤔 Assuming that is the case, I would advocate keeping the slug as invisible as possible. IE auto-generating If there are flows in which the end user needs to interact with the slug, then let's list them and think about where/when it might be best to edit. |
Only case I can think of is this. Where we can't auto-generate slugs because of non-Latin chars. Generally we want the slug field hidden. |
Mamaduka
added
Backport to WP Minor Release
Mamaduka
added a commit
that referenced
this pull request
Feb 16, 2022
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Resolves #38160.
Currently, templates and template parts don't allow non-Latin characters as a slug.
This PR tries to "clean" up the slug first, and if this isn't possible, use a fallback slug.
Testing Instructions
私のテンプレートパーツのテストorქართული ნაწილი.Types of changes
Bugfix
Checklist:
*.native.jsfiles for terms that need renaming or removal).