Updates to ChoiceBox; Checkbox, Radio components #1174

jonkafton · 2024-06-26T16:56:20Z

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/4661

Description (What does it do?)

Updates the styles and hover states for the ChoiceBox and its field grid.

Also:

Provides a Checkbox and Radio in ol-components
Exports styles for reuse (applies to the SearchDisplay course-search-utils overrides).
Adds Storybook stories for ChoiceBox.
Removes the hidden input control in ChoiceBox (accessibility concern) to style directly.
Upgrades Storybook.

Screenshots (if appropriate):

How can this be tested?

While authenticated, navigate to /onboarding and step through the onboarding screens.
See also the ChoiceBox, ChoiceBoxField and Checkbox stories in Storybook
- /?path=/story/ol-components-choicebox--checkbox
- /?path=/story/ol-components-choiceboxfield--checkbox
- /?path=/story/ol-components-checkbox--simple

…y/course-search-utils

ChristopherChudzicki · 2024-06-27T14:37:16Z

frontends/ol-components/src/components/ChoiceBox/ChoiceBoxField.tsx

+  const fieldGridItemProps = {
+    ...gridItemProps,
+  }


out of curiosity, any reason to spread this?

Nope 🤷‍♂️

ChristopherChudzicki · 2024-06-27T14:40:54Z

frontends/ol-components/src/components/Radio/Radio.tsx

+  )
+}
+
+Radio.styles = containerStyles


Is this for re-use? I had no idea typescript would let you do this without declaring the property on Radio beforehand. Interesting.

Suggestion: wrap it in css() from import { css } from "@emotion/react". That will allow it to work with either the object or template literal syntax.

Nice. It won't work outside of Emotion though if we need it as plain css, which I guess we don't (ie. can't be interpolated - its toString() method returns a warning message).

ChristopherChudzicki · 2024-06-27T15:25:03Z

frontends/ol-components/src/components/Checkbox/Checkbox.stories.tsx

+  const handleChange = (event: ChangeEvent<HTMLInputElement>) => {
+    setChecked(event.target.checked)
+  }


**Suggestion: ** call action("changed")(event)

from import { action } from "@storybook/addon-actions"

ChristopherChudzicki · 2024-06-27T15:25:12Z

frontends/ol-components/src/components/ChoiceBox/ChoiceBox.stories.tsx

+  const handleChange = (event: ChangeEvent<HTMLInputElement>) => {
+    setChecked(event.target.checked)
+  }


**Suggestion: ** call action("changed")(event)

from import { action } from "@storybook/addon-actions"

ChristopherChudzicki · 2024-06-27T15:25:19Z

frontends/ol-components/src/components/ChoiceBox/ChoiceBoxFieldCheckbox.stories.tsx

+      const target = event.target as HTMLInputElement
+      if (target.checked) {


**Suggestion: ** call action("changed")(event)

from import { action } from "@storybook/addon-actions"

Good point. I achieved the same with this on the meta:

argTypes: { onChange: { action: "change" } }

and calling props.onChange?.(event) in our StateWrapper.

ChristopherChudzicki · 2024-06-27T15:25:33Z

frontends/ol-components/src/components/Radio/Radio.stories.tsx

+  const [checked, setChecked] = useState(!!props.checked)
+
+  const handleChange = (event: ChangeEvent<HTMLInputElement>) => {
+    setChecked(event.target.checked)


**Suggestion: ** call action("changed")(event)

from import { action } from "@storybook/addon-actions"

ChristopherChudzicki · 2024-06-27T15:31:13Z

frontends/ol-components/src/components/ChoiceBox/ChoiceBoxField.tsx

+  return (
+    <ChoiceBoxField
+      type="checkbox"
+      isChecked={(choice) => values?.indexOf(choice.value) !== -1}


values?.includes(...) 🤷

ChristopherChudzicki · 2024-06-27T15:35:09Z

frontends/ol-components/src/components/ChoiceBox/ChoiceBoxField.tsx

+  )
+}
+
+export { ChoiceBoxField, CheckboxChoiceBoxField, RadioChoiceBoxField }


My inclination would be to not export ChoiceBoxField, at least for now.

May also want to add a docstring to to ChoiceBoxField along lines of

/** * Prefer using `CheckboxChoiceBoxField` or `RadioChoiceBoxField` instead of * this component, where possible. */

The only case I can think of where you'd use ChoiceBoxField directly would be if you needed a checkbox with the "partially checked" state?

True. We need the wrapping components for the different isChecked workings.

The naming Field isn't great here. Technically they're fieldsets. Thoughts on RadioChoiceBoxFieldset or even RadioChoiceBoxGrid?

IMO... "Field" is fine. "Field" is not a technical HTML term as far as I know. (In contrast to fieldset—please educate me if I am mistaken!). *I.e., I don't equate "field" with "single input element".

RadioGroup is a good name for the radio version (after `role="radiogroup"—which we could add, but isn't really necessary afaik since we are using native radio elements). But there's not checkboxgroup role.

Leaving *Field seems fine to me.

I see it as an individual form field (ie. an input) as opposed to a fieldset containing multiple - the ChoiceBoxField container element is an html fieldset.

ChristopherChudzicki · 2024-06-27T16:06:47Z

frontends/ol-components/src/components/ChoiceBox/ChoiceBoxField.tsx

+      <FormLabel component="legend" sx={{ width: "100%" }}>
+        {label}
+      </FormLabel>


A few overall comments here:

It would be nice to be able to support required and an error message.

I'm not sure what FormControl gives us. My understanding is that MUI uses this to wrap its inputs to manage some internal state. See https://mui.com/base-ui/react-form-control/. I chose not to use it for our TextField, and instead we have FormFieldWrapper in FormHelpers.

That said, FormFieldWrapper isn't directly directly usable here. It's expecting a single input, like select or text.

That said, the error message / description / required components in FormHelpers might be helpful.

I don't think we have very explicit component designs for ChoiceBoxField, but it seems to me it should match https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=4345-87576&m=dev :

I.e., the legend currently doesn't have the style we want.

We do have explicit field designs for text input. See https://www.figma.com/design/Eux3guSenAFVvNHGi1Y9Wm/MIT-Design-System?node-id=104-1582&m=dev

My inclination for actionable steps right now:

Make title match profile page style (i.e., subtitle2, which is what the textfield and selectfield labels use, too)

add spot for overall description + error message, using styles from FormHelpers. You could alter FormFieldWrapper and use it, if you see fit. But the spacing between legend and input is also a little different, I think. 8px not 4px.

ChristopherChudzicki · 2024-06-27T16:07:55Z

frontends/ol-components/src/components/ChoiceBox/ChoiceBoxField.tsx

+interface ChoiceBoxFieldProps extends BaseChoiceBoxFieldProps {
+  type: ChoiceBoxProps["type"]
+  name?: string
+  isChecked: (choice: ChoiceBoxChoice) => boolean


Comment: I can imagine isDisabled would also be useful, but I don't think we need to add it now.

ChristopherChudzicki · 2024-06-27T16:37:59Z

Adding this as a requested change... I do think we should do something about this alignment issue:

I manually edited a title, there, but note alignment on Figma dashboard page.

ChristopherChudzicki

Requested change is the alignment issue.

jonkafton · 2024-06-27T20:22:41Z

These changes also pushed:

Style the title (fieldset legend) to match designs
Remove the label+input center alignment and restyle so the input control remains anchored top for multiline labels

ChristopherChudzicki

👍

jonkafton added 7 commits June 25, 2024 19:06

Refactor out ChoiceBoxField

b89b836

Positioning styles

fa11659

ChoiceBox stories

9b3527c

Field stories

34cb76b

Default grid spacing

8002b01

Update hover styles

316d72d

Checkbox ol-component. Reuse in ChoiceBox and styles for SearchDispla…

939eb41

…y/course-search-utils

jonkafton added the Work in Progress label Jun 26, 2024

ChristopherChudzicki self-assigned this Jun 26, 2024

jonkafton added 2 commits June 26, 2024 20:18

Upgrade storybook

11bcf5f

Radio input ol-component

b927b01

jonkafton temporarily deployed to github-pages June 26, 2024 18:21 — with GitHub Actions Inactive

Name prop optional

bcbb82e

jonkafton temporarily deployed to github-pages June 26, 2024 18:34 — with GitHub Actions Inactive

ChristopherChudzicki removed their assignment Jun 26, 2024

jonkafton added 2 commits June 26, 2024 20:52

Fixes issue with hover styled on inner input while checked

a4ff4ca

Fixes inner input losing hover state while out label hovered

4b4c186

jonkafton added Needs Review An open Pull Request that is ready for review Work in Progress and removed Work in Progress Needs Review An open Pull Request that is ready for review labels Jun 26, 2024

Revert to using mui radio input here

d081090

jonkafton temporarily deployed to github-pages June 26, 2024 19:32 — with GitHub Actions Inactive

Lint complaint

11e7617

jonkafton temporarily deployed to github-pages June 26, 2024 19:41 — with GitHub Actions Inactive

jonkafton temporarily deployed to github-pages June 26, 2024 19:45 — with GitHub Actions Inactive

Merge branch 'main' into jk/4661-choicebox

a3f2d16

jonkafton temporarily deployed to github-pages June 26, 2024 20:05 — with GitHub Actions Inactive

Merge branch 'main' into jk/4661-choicebox

c215898

jonkafton changed the title ~~Updates to ChoiceBox and a Checkbox component~~ Updates to ChoiceBox; Checkbox, Radio components Jun 26, 2024

jonkafton temporarily deployed to github-pages June 26, 2024 20:08 — with GitHub Actions Inactive

jonkafton added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Jun 26, 2024

ChristopherChudzicki self-assigned this Jun 27, 2024

ChristopherChudzicki reviewed Jun 27, 2024

View reviewed changes

Convert styles to @emotion/css objects

1b8e660

jonkafton temporarily deployed to github-pages June 27, 2024 18:40 — with GitHub Actions Inactive

Styled form label

ef6d550

jonkafton temporarily deployed to github-pages June 27, 2024 19:12 — with GitHub Actions Inactive

jonkafton added 2 commits June 27, 2024 22:05

Re-style for top right anchored input control

efd2594

Stylelint vs prettier

808ba01

jonkafton temporarily deployed to github-pages June 27, 2024 20:14 — with GitHub Actions Inactive

jonkafton requested a review from ChristopherChudzicki June 27, 2024 20:22

ChristopherChudzicki approved these changes Jun 27, 2024

View reviewed changes

ChristopherChudzicki added Waiting on author and removed Needs Review An open Pull Request that is ready for review labels Jun 27, 2024

jonkafton merged commit 38eb61c into main Jun 28, 2024

jonkafton deleted the jk/4661-choicebox branch June 28, 2024 12:26

This was referenced Jul 1, 2024

Release 0.13.17 #1209

Closed

Release 0.13.17 #1214

Merged

		const target = event.target as HTMLInputElement
		if (target.checked) {

Updates to ChoiceBox; Checkbox, Radio components #1174

Updates to ChoiceBox; Checkbox, Radio components #1174

Uh oh!

Conversation

jonkafton commented Jun 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What are the relevant tickets?

Description (What does it do?)

Screenshots (if appropriate):

How can this be tested?

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jonkafton Jun 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristopherChudzicki commented Jun 27, 2024

Uh oh!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

jonkafton commented Jun 27, 2024

Uh oh!

ChristopherChudzicki left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jonkafton commented Jun 26, 2024 •

edited

Loading

jonkafton Jun 27, 2024 •

edited

Loading