-
Notifications
You must be signed in to change notification settings - Fork 1k
Feat/add internationalization select multiple component #7009
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/add internationalization select multiple component #7009
Conversation
8dbe26e
to
01b90ba
Compare
23e913b
to
0bbb2f7
Compare
0bbb2f7
to
9a554be
Compare
…/add_internationalization_selectMultiple_Component
Hey, thanks for your help @britt6612 @jcfilben :), I struggle with the snapshot test I really dont know when and how to update theme cleanly... |
Good news, the tests are fixed, let me know if there's something else to do |
Great we will plan on reviewing this week! Glad you got the tests fixed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some initial comments
src/js/components/DataFilter/__tests__/__snapshots__/DataFilter-test.tsx.snap
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Dropped a few naming suggestions!
Just pushed a bunch of commits to update the messages keys :) Waiting on further comments! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one final request thank you for your patience with us :)
Will be a little easier to go through when they are in alphabetical order.
I think we're good on the alphabetical order :) I left one thread open to be sure we agree on it.
you're very welcome, I'm learning a lot ! |
Just one more file left to organize which is
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ee32426
to
0ff56aa
Compare
Thanks so much for the work here @tol-any. As I reviewed, a few more refinements came up. I didn't want them to get lost in review comments, so for clarity I've filed a separate PR which branches off of yours. The latest commit includes my refinements: df0d7be I couldn't tag you as a reviewer, but would you like to take a look to make sure all looks good? I've tagged others for final review as well. |
Superseded by #7104 |
What does this PR do?
Fixing the Issue #6337
It's a follow-up to #6655 (comment)