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(core,common,admin-ui): Implement normalizeString function using slug library #2721

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

andriinuts
Copy link
Contributor

Description

Updated normalizeString to use slug npm package which fixes issues with multilanguage slug generation and especially Cyrillic letters: https://discord.com/channels/1100672177260478564/1208130249632649256

Breaking changes

Updated all calls of the normalizeString function to include a third parameter for language code. This enhancement allows the function to normalize strings based on specific language rules.

Checklist

📌 Always:

  • I have set a clear title
  • My PR is small and contains a single feature
  • I have checked my own PR

👍 Most of the time:

  • I have added or updated test cases
  • I have updated the README if needed

Copy link

netlify bot commented Mar 7, 2024

Deploy Preview for effervescent-donut-4977b2 ready!

Name Link
🔨 Latest commit 971658e
🔍 Latest deploy log https://app.netlify.com/sites/effervescent-donut-4977b2/deploys/65e9f5af6484580008023b85
😎 Deploy Preview https://deploy-preview-2721--effervescent-donut-4977b2.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@michaelbromley
Copy link
Member

Hi,

This is basically good and I like the slug lib since it is small and has zero dependencies. The main issue I see is the way it is handling non-supported languages, e.g. Chinese. Right now it is replacing any Chinese string with '', which is also why the tests are failing.

In the Discord chat about this, a Chinese user said he would in any case use English for the slugs, but of course it is not feasible to translate Chinese to English in a slug lib!

So my preferred behaviour would be to leave the chinses as-is, rather than replacing with empty string. For the Admin UI this is not a big issue since the admin can directly edit the slug before saving. But for things like imports, we could run into the problem.

Do you have any ideas on this?

@andriinuts
Copy link
Contributor Author

andriinuts commented Mar 19, 2024

Hi,

This is basically good and I like the slug lib since it is small and has zero dependencies. The main issue I see is the way it is handling non-supported languages, e.g. Chinese. Right now it is replacing any Chinese string with '', which is also why the tests are failing.

In the Discord chat about this, a Chinese user said he would in any case use English for the slugs, but of course it is not feasible to translate Chinese to English in a slug lib!

So my preferred behaviour would be to leave the chinses as-is, rather than replacing with empty string. For the Admin UI this is not a big issue since the admin can directly edit the slug before saving. But for things like imports, we could run into the problem.

Do you have any ideas on this?

Hi, I agree. But for slug I haven't found a solution on how to leave just Chinese characters. I guess for this we have to add all characters to the charmap the same as I did for the - like: slug.charmap['-'] = '-'; or slug.extend({'-': '-'}) but Chinese json will be huge (we will replace English to the same character, what I don't like too much).
Or maybe we can detect the Chinese language by regexp /[\u4e00-\u9fa5]/.test(text) and use your previous logic for this case and slug for all other languages.
What do you think?

@andriinuts
Copy link
Contributor Author

andriinuts commented Apr 17, 2024

@michaelbromley do you have any thoughts?

@michaelbromley
Copy link
Member

@andriinuts Yes, if we can have the benefit of 'slug' for the western languages, and no-op for non-supported languages like Chinese, then this is a good approach.

@dlhck dlhck added this to the v3.3.0 milestone Sep 27, 2024
@dlhck dlhck added status: research needed 🔍 More in-depth research need to make a decision+ type: feature ✨ labels Sep 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: research needed 🔍 More in-depth research need to make a decision+ type: feature ✨
Projects
Status: 📅 Planned
Development

Successfully merging this pull request may close these issues.

3 participants