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

USWDS - Select: Include standard "multiple" stylings. Resolves #4423 #4766

Merged
merged 9 commits into from
Jul 5, 2022

Conversation

mahoneycm
Copy link
Contributor

@mahoneycm mahoneycm commented Jun 3, 2022

Preview

Select →

Description

Resolves #4423

According to the HTML 5 standard, a select element whose multiple attribute is present is "expected to render as an inline-block box whose height is the height necessary to contain as many rows for items as given by the element's display size, or four rows if the [size] attribute is absent." This standard behavior has the benefit of making it obvious that multiple selections can be made and users have come to expect and depend on this visual clue. All of the major browsers implement this behavior by default. The USWDS component system suppresses this behavior, ignoring the size attribute as well as the standard's instructions for the height of the box in the absence of that attribute.

This PR

  • Adds expected styling for select when the multiple attribute is present
  • Sets default styling as described by HTML 5 standard
  • Adds storybook controls to enable the multiple attribute and set size attribute
  • If size = 0 it will act as if the size attribute is absent and do the standard default of four rows.

Additional information

Standard select:

image

Multiple Enabled:

image

New Storybook controls:

image

Before you hit Submit, make sure you’ve done whichever of these applies to you:

  • Follow the 18F Front End Coding Style Guide and Accessibility Guide.
  • Run npm test and make sure the tests for the files you have changed have passed.
  • Run your code through HTML_CodeSniffer and make sure it’s error free.
  • Title your pull request using this format: [Website] - [UI component]: Brief statement describing what this pull request solves.

- Turning Multiple to true adds multiple and size attributes to markdown (twig)
- Setting size to 0 defaults in 4 row height as standard for multiple select when size is absent
Copy link
Contributor

@amyleadem amyleadem left a comment

Choose a reason for hiding this comment

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

Looks like this is working well!
A couple of thoughts:

  • We should have a separate story in Storybook for when multiple is true

  • The padding on the select box can cause text to get cut off if the size is less than the item count. It might be better to add the padding to the options when multiple is true.
    image

  • Additionally, we have this note in our docs for select:
    "When to consider something else: Multi-select. If you need to allow users to choose more than one option at once. Users often don’t understand how to choose multiple items from select elements. Use checkboxes instead."

    • Are we now allowing multiple select as a recommended practice? If this is now an acceptable pattern, we should remove this line, provide use-case recommendations, and should add some notes about both multiple and size in our documentation.

Let me know if you have questions!

@mahoneycm
Copy link
Contributor Author

Thanks @amyleadem!

  • Having a second story could be a good idea! I feel like it also depends on if we want this advertised as an option or not, because of the checklist note you mentioned
  • I'm open to suggestions on how to handle the padding. I initially left it because when it is missing, it becomse a little less clear that you can scroll to see more options (i.e. if size is not set and it is default to 4 rows, but there are more than four options):

image

(of course the scroll bar is a good indicator)

  • I also asked Dan about the guidance re: checkboxes but the issue is that the multiple select is a native functionality that the HTML 5 standards require. Dan said the following: "So yes, I think that in most instances, checkboxes are preferable to multi-select. Multi-select is bad UI, I’d say, like 99.99% of the time. That said, native multi-select should work with USWDS and should probably work as the user suggests."

@mejiaj
Copy link
Contributor

mejiaj commented Jun 10, 2022

The padding on the select box can cause text to get cut off if the size is less than the item count. It might be better to add the padding to the options when multiple is true.

I think the padding is coming from the default input styles. It seems expected to leave it in by default for this.

I agree with Dan's comments. We're not recommending this as a new pattern, we're just making sure USWDS styles don't break the expected behavior. We also don't have research to suggest otherwise, so we can leave guidance as-is. Good consideration though!

I agree with @amyleadem, a separate story would be better.

- Restores select to it's original settings
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Thanks Charlie, I've added some feedback.

packages/usa-select/src/styles/_usa-select.scss Outdated Show resolved Hide resolved
<option value="value3">Option G</option>
</select>
</form>

Copy link
Contributor

Choose a reason for hiding this comment

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

Can we move this to a directory usa-select/test/test-patterns?

Similar to what we did with this component test template
https://github.com/uswds/uswds/blob/1db0e68d7e5cf21ae52f2d8a29d5963c7aa1b681/packages/usa-combo-box/src/test/test-patterns/test-usa-combo-box.twig

packages/usa-select/src/usa-select.json Outdated Show resolved Hide resolved
@@ -1,9 +1,14 @@
import Component from "./usa-select.twig";
import Multiple from "./usa-select--multiple.twig";
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's create a test section in Storybook for this, see combobox example
https://github.com/uswds/uswds/blob/cm-select-multiple/packages/usa-combo-box/src/usa-combo-box.stories.js

- Creates it's own twig file with defaults set
- Deletes now unused files
@mahoneycm mahoneycm requested a review from mejiaj June 13, 2022 20:48
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Tiny changes, otherwise LGTM!

packages/usa-select/src/usa-select.stories.js Outdated Show resolved Hide resolved
packages/usa-select/src/usa-select.twig Outdated Show resolved Hide resolved
@mahoneycm mahoneycm requested a review from mejiaj June 14, 2022 13:54
@mejiaj mejiaj requested a review from amyleadem June 17, 2022 19:47
Copy link
Contributor

@amyleadem amyleadem 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 to me!

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.

The select element is not rendered properly when "multiple" is specified
4 participants