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
@ankenu
Copy link

ankenu commented Nov 15, 2024

Hi, are there any changes?

@asonnleitner
Copy link
Contributor

I'd recommend considering slugify instead of slug for this change. Here's why:

  1. It has better developer experience with TypeScript typings out of the box
  2. Smaller bundle size impact
  3. Much wider adoption in the ecosystem (higher weekly downloads, more GitHub stars)
  4. Supports both ESM and CommonJS, making it more flexible for different project setups

The full comparison of differences is documented here: https://github.com/Trott/slug#differences-between-slug-and-slugify-packages

Some notable differences in behavior to consider:

  • slugify preserves casing by default (can be configured)
  • It maps symbols to words rather than removing them (e.g. $100dollar100)
  • Returns empty string for invalid inputs instead of generating hashes

These behaviors might actually be preferable depending on the use case. What are your thoughts on making this switch?

@michaelbromley
Copy link
Member

$100 → dollar100

This behavior I find probably undesirable, but I think different people will have different opinions on that.

What will slugify do with Chinese characters, for example? Do we have the same issue there as with slug?

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.

5 participants