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

refactor(locale): lowercase Mexican color names #3200

Merged
merged 3 commits into from
Nov 9, 2024
Merged

Conversation

ST-DDT
Copy link
Member

@ST-DDT ST-DDT commented Oct 20, 2024

Fixes #2841


According to chatgpt Spanish color names are spelled using lowercase.
So this PR fixes the Mexican color names.

@ST-DDT ST-DDT added p: 1-normal Nothing urgent c: locale Permutes locale definitions m: color Something is referring to the color module labels Oct 20, 2024
@ST-DDT ST-DDT added this to the vAnytime milestone Oct 20, 2024
@ST-DDT ST-DDT requested review from a team October 20, 2024 16:56
@ST-DDT ST-DDT self-assigned this Oct 20, 2024
Copy link

netlify bot commented Oct 20, 2024

Deploy Preview for fakerjs ready!

Name Link
🔨 Latest commit 51d39bb
🔍 Latest deploy log https://app.netlify.com/sites/fakerjs/deploys/672f3824d33a7800085ddef2
😎 Deploy Preview https://deploy-preview-3200.fakerjs.dev
📱 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.

Copy link

codecov bot commented Oct 20, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.96%. Comparing base (5832417) to head (51d39bb).
Report is 1 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #3200      +/-   ##
==========================================
- Coverage   99.96%   99.96%   -0.01%     
==========================================
  Files        2805     2805              
  Lines      217115   217115              
  Branches      977      976       -1     
==========================================
- Hits       217049   217036      -13     
- Misses         66       79      +13     
Files with missing lines Coverage Δ
src/locales/es_MX/color/human.ts 100.00% <100.00%> (ø)

... and 2 files with indirect coverage changes

@matthewmayer
Copy link
Contributor

I'd be happier if we can get some actual Spanish speakers to review. Maybe we can ping some users who contributed to Spanish locale in the past?

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 22, 2024

@rasputino I have seen you have contributed some other Spanish translations as well.
Are you available for a short review of color names?

@rasputino
Copy link
Contributor

Sure! I'll take a look at it this weekend.

@rasputino I have seen you have contributed some other Spanish translations as well.
Are you available for a short review of color names?

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 22, 2024

Awesome ❤️

rasputino added a commit to rasputino/faker that referenced this pull request Oct 27, 2024
@rasputino
Copy link
Contributor

@ST-DDT. @matthewmayer

I'm sorry, but I believe the names are not correct; we shouldn't rely so much on ChatGPT. There are colors like "almendra" ("almond") that simply aren't used or are too "fanciful" to be considered a color.

Also, in Mexico, although there are very few differences and it would be perfectly understood, there are colors that are commonly referred to differently. For example, the color "naranja" (orange) in Mexico would be called "anaranjado." Therefore, IMO, it's not correct to delete the file of colors in Mexican Spanish.

I'm making this proposal to improve and expand the colors in Spanish from Spain (Castilian). For the Mexican version, it should be reviewed by a native from there.

7dcac93

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 27, 2024

@rasputino Would you like to create a PR or should I merge your commit into this one?

@rasputino
Copy link
Contributor

@rasputino Would you like to create a PR or should I merge your commit into this one?

What do you prefer?

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 27, 2024

Due to maintainer availability and review rules, it will likely be merged faster if you create a new PR.

rasputino added a commit to rasputino/faker that referenced this pull request Oct 27, 2024
@rasputino
Copy link
Contributor

Due to maintainer availability and review rules, it will likely be merged faster if you create a new PR.

Created: #3230

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 27, 2024

@rasputino Just for clarification. Is the Mexican color list

  • good enough as is,
  • or should it all be lowercase,
  • or should we still remove it to fall back to Spanish?

@rasputino
Copy link
Contributor

@rasputino Just for clarification. Is the Mexican color list

  • good enough as is,
  • or should it all be lowercase,
  • or should we still remove it to fall back to Spanish?
  • It should be in lowercase.

Don’t remove it. It should be reviewed by a Mexican. I can ask some of my Mexican friends if you'd like me to provide the corrections.

@ST-DDT
Copy link
Member Author

ST-DDT commented Oct 27, 2024

I can ask some of my Mexican friends if you'd like me to provide the corrections.

That would be great.

@ST-DDT ST-DDT changed the title refactor(locale): improve Spanish color names refactor(locale): lowercase Mexican color names Nov 2, 2024
@ST-DDT
Copy link
Member Author

ST-DDT commented Nov 2, 2024

@rasputino I updated this PR to switch to lowercase Mexican color names as a basic fix for the reported issue.
If one of your friends could provide a new set of Mexican color names, then I will close this PR in favor of the new additions.

@ST-DDT ST-DDT merged commit 0d85075 into next Nov 9, 2024
23 checks passed
@ST-DDT ST-DDT deleted the locale/es/color branch November 9, 2024 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c: locale Permutes locale definitions m: color Something is referring to the color module p: 1-normal Nothing urgent
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Check capitisation of ex_MX/color/human
3 participants