Skip to content

Conversation

tol-any
Copy link
Contributor

@tol-any tol-any commented Nov 7, 2023

What does this PR do?

Fixing the Issue #6337

It's a follow-up to #6655 (comment)

@tol-any tol-any force-pushed the feat/add_internationalization_selectMultiple_Component branch from 8dbe26e to 01b90ba Compare November 7, 2023 07:58
@tol-any tol-any force-pushed the feat/add_internationalization_selectMultiple_Component branch 2 times, most recently from 23e913b to 0bbb2f7 Compare November 7, 2023 08:54
@tol-any tol-any force-pushed the feat/add_internationalization_selectMultiple_Component branch from 0bbb2f7 to 9a554be Compare November 7, 2023 09:14
@jcfilben jcfilben added the PRty Biweekly PR reviews by grommet team with hoping of closing such PRs label Nov 8, 2023
…/add_internationalization_selectMultiple_Component
@tol-any
Copy link
Contributor Author

tol-any commented Nov 10, 2023

Hey, thanks for your help @britt6612 @jcfilben :), I struggle with the snapshot test I really dont know when and how to update theme cleanly...

@tol-any
Copy link
Contributor Author

tol-any commented Nov 10, 2023

Good news, the tests are fixed, let me know if there's something else to do

@britt6612
Copy link
Collaborator

britt6612 commented Nov 13, 2023

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

Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some initial comments

Copy link
Collaborator

@taysea taysea left a 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!

@tol-any
Copy link
Contributor Author

tol-any commented Jan 22, 2024

Just pushed a bunch of commits to update the messages keys :)

Waiting on further comments!

Copy link
Collaborator

@britt6612 britt6612 left a 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.

@tol-any
Copy link
Contributor Author

tol-any commented Jan 23, 2024

I think we're good on the alphabetical order :) I left one thread open to be sure we agree on it.

thank you for your patience with us :)

you're very welcome, I'm learning a lot !

@britt6612
Copy link
Collaborator

Just one more file left to organize which is Grommet/propTypes.js

    messages: PropTypes.shape({
      clearAll: PropTypes.string,
      clearAllA11y: PropTypes.string,
      onMore: PropTypes.string,
      open: PropTypes.string,
      optionNotSelected: PropTypes.string,
      optionSelected: PropTypes.string,
      search: PropTypes.string,
      selectAll: PropTypes.string,
      selectAllA11y: PropTypes.string,
      selectDropDown: PropTypes.string,
      selected: PropTypes.string,
      selectedOfTotal: PropTypes.string,
      selectedOptions: PropTypes.string,
      summarizedValue: PropTypes.string,
    }),

Copy link
Collaborator

@britt6612 britt6612 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@britt6612 britt6612 requested review from jcfilben and taysea January 24, 2024 15:47
Copy link
Collaborator

@jcfilben jcfilben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the showSelectedInline version of SelectMultiple I am no longer seeing the 'x of y selected' message when I have options selected and the drop is closed. See the SelectMultiple/showSelectedInline storybook example.

Screen Shot 2024-01-26 at 12 07 26 PM Screen Shot 2024-01-26 at 12 10 07 PM

@tol-any tol-any force-pushed the feat/add_internationalization_selectMultiple_Component branch from ee32426 to 0ff56aa Compare January 29, 2024 08:31
@taysea taysea mentioned this pull request Jan 29, 2024
3 tasks
@taysea
Copy link
Collaborator

taysea commented Jan 29, 2024

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.

@jcfilben
Copy link
Collaborator

jcfilben commented Feb 1, 2024

Superseded by #7104

@jcfilben jcfilben closed this Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PRty Biweekly PR reviews by grommet team with hoping of closing such PRs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants