Skip to content

Conversation

jcfilben
Copy link
Collaborator

@jcfilben jcfilben commented May 25, 2023

What does this PR do?

This PR changes the 'no options found' container in Select and SelectMultiple so that it is no longer a disabled Button and is instead just Text within a Box. It also adds theme.select.emptySearchMessage.container and theme.select.emptySearchMessage.text to allow styling of the emptySearchMessage. This theme structure mirrors that of theme.select.options.

For backwards compatibility, if no theme.select.emptySearchMessage object exists, we'll fall back to theme.select.option styling.

Where should the reviewer start?

src/js/components/Select/EmptySearchOption.js

What testing has been done on this PR?

For HPE theme, the following configuration could be used in the theme:

const option = {
  color: 'text',
  border: {
    radius: '0px',
  },
  pad: {
    horizontal: '12px',
    vertical: '6px',
  },
  font: {
    weight: 500,
  },
};
...
const hpe = {
  button: {
    option,
  },
  select: {
    emptySearchMessage: {
      container: {
        pad: option.pad,
      },
    },
  },
};

How should this be manually tested?

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?

What are the relevant issues?

Closes #6821

Screenshots (if appropriate)

Do the grommet docs need to be updated?

yes

Should this PR be mentioned in the release notes?

yes

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

backwards compatible

@jcfilben jcfilben requested review from halocline and MikeKingdom May 25, 2023 22:24
Comment on lines +393 to +394
{options.length > 0 ? (
<OptionsContainer
Copy link
Collaborator

Choose a reason for hiding this comment

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

EmptySearchOption is also used in SelectMultipleContainer.js in a similar way. Do you need to pull it out of the container there also?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call, I moved the EmptySearchOption outside of the options container in SelectMultipleContainer.js

Copy link
Collaborator

@MikeKingdom MikeKingdom left a comment

Choose a reason for hiding this comment

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

It looks great and seems to work well. I'm mainly curious about if the SelectMultiple case needs to pull the EmptySearchOption out of the OptionsContainer also.

Copy link
Collaborator

@MikeKingdom MikeKingdom left a comment

Choose a reason for hiding this comment

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

Looks good!

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.

Select - empty search message accessibility
4 participants