Skip to content

Commit

Permalink
Reimplement <Select> using Headless UI
Browse files Browse the repository at this point in the history
## Description

I put the new implementation in `src/components`, the idea being this
is where new self-contained UI components can go. `src/select.tsx` is
not used by the new calculator, so it will go away when the old
calculator does.

This conforms to the design system with a few known differences:

- When the Select is open, it doesn't have the purple outline. If
  you're using the keyboard, the _listbox_ gets the purple
  outline. This is because Headless UI focuses the listbox when you're
  navigating it. It is possible to remove the outline from the listbox
  and have the Select's button outlined when it's open, but this
  causes a slight flicker when the listbox closes.

- The Selects should be 48px tall, but they're currently 46px to match
  the text input fields. I'll fix that separately.

I used Floating UI to position the list so it's within the viewport.
It doesn't move when you scroll the page -- I don't think that's
really necessary -- but it's trivial to add if people want.

### Accessibility

One notable change I made for a11y is removing the X buttons in the
multiselect's tags. These tripped the "nested interactable elements"
check -- a button (the X) within a button (the whole control). The
Shoelace implementation was not hitting this because the top-level
element is a div, not a button. We could restore the X button and do
some workaround to get around the check, but it really is a nested
interactable, and that's an annoying experience for screenreader
users. Not sure what to do there. I don't think losing the Xs is a
major loss, but could be convinced otherwise.

I also removed the tooltip for the "Projects you're interested in"
field, because it just said "select the projects you're interested
in", which is pointless.

## Test Plan

Use the element with mouse and keyboard.

- Click the labels and make sure they open the element (this is a
  difference from Shoelace; those labels would only focus the element,
  not open it). Click the tooltip icons and make sure they open the
  tooltip and don't focus the element.

- With the multiselect, click the X buttons to make sure those deselect
  the corresponding option.

- Make the window just over 640px wide to make the multiselect super
  narrow, and have HVAC selected, along with something alphabetically
  later. This is the worst-case scenario for shrinking the element.

- Change the zip code and observe the loading state in the utility
  selector.

- With VoiceOver, make sure each form element has the label and
  current contents announced, with no extraneous bits announced (like
  icons).

  Honestly I'm not sure how good this screenreader experience is,
  although it does seem cleaner than what Shoelace gave us.

Do all of the above in Chrome, Safari, Firefox, on narrow and wide
layouts. Also do the non-VO stuff in MobileSafari.
  • Loading branch information
oyamauchi committed Jan 26, 2024
1 parent 6eefa72 commit a75ec48
Show file tree
Hide file tree
Showing 13 changed files with 450 additions and 118 deletions.
5 changes: 4 additions & 1 deletion cypress/e2e/state-calculator-events.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,10 @@ describe('rewiring-america-state-calculator events', () => {
expect(event.detail).to.exist;
expect(event.detail.formData).to.exist;
expect(event.detail.formData.zip).to.equal('02859');
expect(event.detail.formData.projects).to.eql(['ev', 'hvac']);
expect(Array.from(event.detail.formData.projects).sort()).to.eql([
'ev',
'hvac',
]);
});
});

Expand Down
17 changes: 3 additions & 14 deletions cypress/e2e/state-calculator.cy.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ describe('rewiring-america-state-calculator', () => {

cy.get('rewiring-america-state-calculator')
.shadow()
.find('sl-select#utility')
.find('button#utility')
.contains('Rhode Island Energy');

cy.get('rewiring-america-state-calculator')
Expand Down Expand Up @@ -137,8 +137,6 @@ describe('rewiring-america-state-calculator', () => {
});

it('shows an error if you query in the wrong state', () => {
cy.intercept({ pathname: '/api/v1/utilities' }).as('utilitiesFetch');

// Add the state-restricting attribute to the element
cy.get('rewiring-america-state-calculator').invoke('attr', 'state', 'RI');

Expand All @@ -147,23 +145,14 @@ describe('rewiring-america-state-calculator', () => {
.find('#zip')
.type('94306'); // California

cy.wait('@utilitiesFetch');

// This error message is displayed under the utility selector, but it's
// inside a shadow DOM so this won't find it. This is to prove that the
// utility selector error is not what the final assertion below is finding.
cy.get('rewiring-america-state-calculator')
.shadow()
.contains('That ZIP code is not in Rhode Island')
.should('not.exist');

cy.get('rewiring-america-state-calculator')
.shadow()
.find('button#calculate')
.click();

cy.get('rewiring-america-state-calculator')
.shadow()
.contains('That ZIP code is not in Rhode Island.');
.find('#error-msg')
.should('contain.text', 'That ZIP code is not in Rhode Island.');
});
});
6 changes: 3 additions & 3 deletions cypress/support/commands.ts
Original file line number Diff line number Diff line change
Expand Up @@ -88,21 +88,21 @@ Cypress.Commands.add(
(projects: string[]) => {
cy.get('rewiring-america-state-calculator')
.shadow()
.find('sl-select#projects')
.find('button#projects')
.click();

projects.forEach(project =>
cy
.get('rewiring-america-state-calculator')
.shadow()
.find(`sl-option[value="${project}"]`)
.find(`li[data-value="${project}"]`)
.click(),
);

// Unfocus the project selector
cy.get('rewiring-america-state-calculator')
.shadow()
.find('sl-select#projects')
.find('button#projects')
.click();
},
);
3 changes: 3 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
"license": "Apache-2.0",
"private": true,
"dependencies": {
"@floating-ui/react-dom": "^2.0.6",
"@headlessui/react": "^1.7.18",
"@lit-labs/task": "^2.0.0",
"@lit/localize": "^0.12.1",
"@shoelace-style/shoelace": "^2.12.0",
Expand Down Expand Up @@ -37,6 +39,7 @@
]
},
"devDependencies": {
"@headlessui/tailwindcss": "^0.2.0",
"@lit/localize-tools": "^0.7.1",
"@parcel/transformer-inline-string": "2.10.2",
"@swc/helpers": "^0.4.14",
Expand Down
6 changes: 3 additions & 3 deletions src/card.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -9,13 +9,13 @@ import clsx from 'clsx';
*/
export const Card = forwardRef<
HTMLDivElement,
PropsWithChildren<{ isFlat?: boolean }>
>(({ isFlat, children }, ref) => (
PropsWithChildren<{ id?: string; isFlat?: boolean }>
>(({ id, isFlat, children }, ref) => (
<div
ref={ref}
id={id}
className={clsx(
'rounded-xl',
'overflow-clip',
'min-w-52',
isFlat && 'bg-yellow-200',
!isFlat && 'bg-white shadow',
Expand Down
32 changes: 32 additions & 0 deletions src/components/form-label.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
import clsx from 'clsx';
import { FC, PropsWithChildren } from 'react';
import { TooltipButton } from '../tooltip';

/**
* Styles the all-uppercase label for a form field. The label element should be
* passed in as the child. Optionally renders a tooltip button after the label.
*/
export const FormLabel: FC<
PropsWithChildren<{
hidden?: boolean;
tooltipText?: string;
}>
> = ({ hidden, tooltipText, children }) => (
<div
className={clsx(
'w-fit',
'flex',
'gap-1',
'my-2',
'text-xsm',
'leading-5',
'font-bold',
'uppercase',
'tracking-[0.55px]',
hidden && 'hidden',
)}
>
{children}
{tooltipText && <TooltipButton text={tooltipText} iconSize={13} />}
</div>
);
Loading

0 comments on commit a75ec48

Please sign in to comment.