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/i18n #8

Merged
merged 5 commits into from
Jul 22, 2021
Merged

Feat/i18n #8

merged 5 commits into from
Jul 22, 2021

Conversation

baymac
Copy link
Owner

@baymac baymac commented Jul 20, 2021

🤔 This is a ...

  • New feature
  • Bug fix
  • Demo update
  • Component style update
  • TypeScript definition update
  • Bundle size optimization
  • Performance optimization
  • Refactoring
  • Code style optimization
  • Test Case
  • README update
  • Other (about what?)

🔗 Related issue link

#1

💡 Background and solution

Problem statement

Add Internalization support

Problem Solving

  1. Added a new localeState which has default value of en locale.
  2. Added selector for a few state to derive their default state based on the localeState value.
  3. Minor fix to Last Day of Month selection.
  4. Added props to Scheduler component to allow custom locale values or reusing an predefined locale in the library.
  5. Added docs

☑️ Self Check before Merge

⚠️ Please check all items below before review. ⚠️

  • Demo in storybook is updated/provided or not needed
  • TypeScript definition is updated/provided or not needed
  • README API section is updated or not needed

@baymac
Copy link
Owner Author

baymac commented Jul 20, 2021

@ShirasawaSama you might want to check this. Let me know if any improvements required.

@ShirasawaSama
Copy link
Contributor

cronstrue can also set the language.

https://github.com/bradymholt/cronstrue#node-1

@ShirasawaSama
Copy link
Contributor

I think the current design is good. Thank you.

But what do you think of the localization API based on material-ui?

https://github.com/mui-org/material-ui/blob/next/packages/material-ui/src/locale/index.ts#L3

@baymac
Copy link
Owner Author

baymac commented Jul 21, 2021

@ShirasawaSama

  1. Yes, I will integrate cronstrue localization as well but it will be limited to the set of languages we provide.

  2. Are you saying that we export locale json and user can import and pass it? Is that it or something else you are trying to say? If that is just the case, I think this way is also fine. Do you think there is any major advantage with mui approach?

@ShirasawaSama
Copy link
Contributor

@baymac If you don't think it's necessary, you don't need to change it.

@baymac
Copy link
Owner Author

baymac commented Jul 22, 2021

@ShirasawaSama I have added locale support for construe, but it is not working for me. Do you have any idea why?

@ShirasawaSama
Copy link
Contributor

@baymac src/components/CronReader.tsx

- import cronstrue from 'cronstrue'
+ import cronstrue from 'cronstrue/i18n'

@baymac
Copy link
Owner Author

baymac commented Jul 22, 2021

@ShirasawaSama thanks. Do you think now this pr is ready to be merged?

@ShirasawaSama
Copy link
Contributor

@baymac yes

@baymac baymac merged commit f880645 into main Jul 22, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants