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

fix: makes SelectMultiple accept undefined or null #6491

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

BrunoViveiros
Copy link
Contributor

What does this PR do?

Makes SelectMultiple accept undefined or null

Where should the reviewer start?

SelectMultiple component

What testing has been done on this PR?

not sure how to test when receiving undefined value

How should this be manually tested?

when passing a undefined value for options it should not generate a error

Do Jest tests follow these best practices?

  • screen is used for querying.
  • The correct query is used. (Refer to this list of queries)
  • userEvent is used in place of fireEvent.
  • asFragment() is used for snapshot testing.

Any background context you want to provide?

I'm not sure if I should create a new story showing a loading when the options are taking some time to fetch

What are the relevant issues?

closes #6351

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

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.

Nice work on this so far! I think it would be good to include a new storybook example that demonstrates one of the loading cases in the original issue.

When options is set to null or undefined the SelectMultiple shows 'No matches found' text at the bottom of the drop. I think we need to adjust the 'No matches found' to only show when we there is actually text in the search input.

@jcfilben jcfilben added the waiting Awaiting response to latest comments label Nov 16, 2022
@jcfilben
Copy link
Collaborator

Hi @BrunoViveiros just wanted to check in on this pull request. I think it is pretty close to the finish line and there are just a few things to tie off. Is this something that you are still interested in working on?

@jcfilben jcfilben added the needs attention To alert grommet team that a PR has been waiting for the author for a while label Jul 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs attention To alert grommet team that a PR has been waiting for the author for a while waiting Awaiting response to latest comments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SelectMultiple - Loading enhancement
2 participants