121 - Add locale functionality and locale mixin #255
Conversation
There was a problem hiding this comment.
@tmcconechy this was a really large PR but from what I can tell the patterns/approach/etc in the context of what we have are really nice!
Hopefully I did not miss anything in reviewing but I put some comments that are mostly just minor where I could.
Again though looks really great for a first PR in the WCs with this and this is a pretty big feature handled this extensively at once 👏
|
|
||
| ```js | ||
| // Respond to parent changing language |
There was a problem hiding this comment.
kind of minor but it seems this should be languagechange? (just from the recent switch to present tense to match DOM)
| @@ -0,0 +1,5 @@ | |||
| // Ids is a JavaScript project, but we define TypeScript declarations so we can | |||
| // confirm our code is type safe, and to support TypeScript users. | |||
|
|
|||
There was a problem hiding this comment.
seems there's props missing on the def but not sure if it's needed since it's an MVP and we haven't tested thoroughly with TS yet.
| @@ -0,0 +1,1783 @@ | |||
| /** | |||
| * @jest-environment jsdom | |||
There was a problem hiding this comment.
for first iteration these tests are really nice/simple/readable. Just for consideration though: it seems like it's quite a lot of repeating similar calls, so was thinking it could be good in future iteration to have a lookup map if that isn't convenient now? e.g. maps of Map<String, Set<String>> with Map<'en' => {'AU', 'GB','IN', 'NZ', 'US', ...}>`
Could make it a little easier to add to or change as well but this also depends on the frequency that clients request changes to this sort of thing here (which I'm not familiar enough to weigh in on).
There was a problem hiding this comment.
I might have to add this for "validating its only in the accepted list in the future. But i think i'll punt on this for now. I sort of tried to have it "open". Although that might not totally make sense in the future. WIll keep this one in mind.
|
@rob2d made changes for all your comments. Good catches. (except i skipping the last one for now) |
Explain the details for making this change. What existing problem does the pull request solve?
This PR adds locale from 4.x but makes some improvements namely:
In addition added some locale features to some but not all components:
This should have most of what we need to move forward.
In addition:
Related github/jira issue (required):
Closes #121
Steps necessary to review your pull request (required):
Translations
Misc
Datagrid Formatters
de-DEand see that the numbers and dates change in the gridarand see that it works RTLRTL
arand see that it works RTLarand see that it works RTL and notice in particular that directional icons change directionIncluded in this Pull Request: