Conversation
| * Displays the select at a specific width. Accepts md or medium (20ex), lg (30ex), xl (40ex). | ||
| */ | ||
| // eslint-disable-next-line i18next/no-literal-string | ||
| @Prop() width: string = 'lg'; |
There was a problem hiding this comment.
note: The enforcement of these specific options (md, lg, & xl) is done by using a dedicated class in the uswds-input-width Sass file.
| /** | ||
| * Whether or not to fire the analytics events | ||
| */ | ||
| @Prop() enableAnalytics?: boolean = false; |
There was a problem hiding this comment.
question The va-select component makes analytics opt-in using this property enableAnalytics. This is standard for all form component to avoid unintentionally leaking PII/PHI.
Is the va-sort component considered a forms component though? If not, then we could make analytics opt-out by changing this to disableAnalytics.
| @@ -1,25 +1,37 @@ | |||
| $system-input-widths: ( | |||
There was a problem hiding this comment.
note: I wrote this as a comment in the file itself but adding it here too for visibility; this Sass variable declaration $system-input-widths is intended to be temporary until we update the USWDS package version.
When USWDS is updated to v3.11.0+, we should instead use:
@use 'uswds-core' as *;...and the $system-input-widths mapping will be available in the uswds-core package directly. As of this writing, the web-components package is using v3.9.0
related issue:
| selectLabel: this.value, | ||
| }, | ||
| }; | ||
| this.componentLibraryAnalytics.emit(detail); |
There was a problem hiding this comment.
note: I will be reaching out to the analytics team to let them know we are adding a new component with analytics. A PR to vets-website will follow.
There was a problem hiding this comment.
Draft vets-website PR:
| @@ -0,0 +1,119 @@ | |||
| import { Component, Host, h, Prop, Event, EventEmitter } from '@stencil/core'; | |||
|
|
|||
There was a problem hiding this comment.
The jsDoc section is missing with the @componentName, @maturityCategory, and @maturityLevel
|
Hi @jamigibbs thanks for taking on this component! Can the spacing between |
|
Hi @jamigibbs! Some additional suggestions to implement as discussed from Standup today:
Also wanted to add 2 other suggestions:
|
|
@derekwang99 Thanks for that feedback! I've made updates to address them and you're welcome to review again. 🙏🏻 |
| // flex-basis instead of max-width in order to adjust the width | ||
| @media (min-width: tokens.$tablet) { | ||
| max-width: none; | ||
| flex-basis: map-get($system-input-widths, $size); |
There was a problem hiding this comment.
note: The sort component has a dedicated class for applying width for two reasons:
- It should only support specific widths
- The container has been changed to
flexto allow for the label to be positioned horizontally to the select field so it needs to useflex-basisto set the initial size of the container.max-widthdoes not work with flex.
| :host .va-select__sort-container { | ||
| @each $size in ('md', 'medium', 'lg', 'xl') { | ||
| &--#{$size} { | ||
| max-width: 100%; |
There was a problem hiding this comment.
note: Mobile web viewports should always display the select as full width so the width class will default to a max-width: 100% here (flex-basis is only needed in desktop). When it gets to the the $tablet breakpoint and larger though, the pre-defined width values will apply.
jeana-adhoc
left a comment
There was a problem hiding this comment.
We should enable axeCheck for the component
| const defaultArgs = { | ||
| 'name': 'options', | ||
| 'value': '', | ||
| 'message-aria-describedby': 'Optional description text for screen readers', |
There was a problem hiding this comment.
Do we need to define this for the storybook stories? I can't think of what this could be. I know it's in the architect doc https://github.com/department-of-veterans-affairs/va.gov-team/blob/master/products/platform/design-system/components/va-sort/properties-architecture.md#properties and we have it in the va-select now. If we don't have to define for for the stories, I'd like not to. If we do, let me know and I'll try to come up with something more realistic.
There was a problem hiding this comment.
@jeana-adhoc Nah, we don't need it. I'll remove it. If you decide to put something more realistic there though, just let me know.
jeana-adhoc
left a comment
There was a problem hiding this comment.
LGTM!
va-sort component checklist complete (🔒 Google Sheet)


Chromatic
https://4637-sort-component--65a6e2ed2314f7b8f98609d8.chromatic.com
Description
This PR will add the new
va-sortweb component.Related tickets and links
Closes department-of-veterans-affairs/vets-design-system-documentation#4637
Screenshots
https://design.va.gov/foundation/breakpoints
Tablet (640px) and above
Below Tablet
Below tablet, width is always 100%
Testing and review
Approvals
See the QA Checklists section below for suggested approvals. Use your best judgment if additional reviews are needed. When in doubt, request a review.
Approval groups
Add approval groups to the PR as needed:
QA checklists
Use the QA checklists below as guides, not rules. Not all checklists will apply to every PR but there could be some overlap.
In all scenarios, changes should be fully tested by the author and verified by the reviewer(s); functionality, responsiveness, etc.
✨ New Component Added
minorlabel🌱 New Component Variation Added
minorlabel🐞 Component Fix
patchlabel♿️ Component Fix - Accessibility
patchlabel🚨 Component Fix - Breaking API Change
majorlabel🔧 Component Update - Non-Breaking API Change
minorlabel📖 Storybook Update
ignore-for-releaselabel🎨 CSS-Library Update
css-librarylabel