Labeled field updates #595

jhp0621 · 2023-08-01T22:07:37Z

Addresses:
#436
#624

Release Notes

Author Checklist

Add unit test(s)
Update version in package.json (see the versioning guidelines)
Update documentation (if necessary)
Add story to storybook (if necessary)
Assign dev reviewer

…adio Group

…oups to preserve html semantics

…r checkbox-group

…into labeled-field-updates

src/forms/inputs/checkbox-group.js

src/forms/labels/labeled-field.js

danparnella

A couple quick thoughts. Will try to add more tonight or tomorrow morning.

src/forms/inputs/checkbox-group.js

src/forms/inputs/dropdown-checkbox-group.js

.storybook/styles/components/_forms.scss

danparnella

Still going through it, but here's a bit more feedback!

.storybook/styles/components/_forms.scss

package.json

src/forms/helpers/dropdown-select.js

src/forms/inputs/checkbox-group.js

chawes13

In addition to adding a migration guide that details the changes that a user would have to make (including updating their stylesheets), we should address the regression in test coverage. Right now on main we're very close to 100%, this PR leaves some pathways uncovered (report available via yarn test).

Main

This branch

.storybook/styles/application.scss

src/forms/inputs/dropdown-checkbox-group.js

.storybook/styles/components/_forms.scss

src/forms/inputs/checkbox-group.js

src/forms/labels/labeled-field.js

stories/forms/inputs/checkbox-group.story.js

test/forms/inputs/radio-group.test.js

test/forms/inputs/dropdown-checkbox-group.test.js

danparnella

Ok, I've gone through everything! Overall, I think this is a really good approach to solve the issue and provides a lot of flexibility. I agree with the comments Conor left and I had a few additional ones added here as well.

src/forms/inputs/radio-group.js

src/forms/inputs/checkbox-group.js

src/forms/inputs/radio-group.js

src/forms/labels/labeled-field.js

test/forms/inputs/checkbox-group.test.js

src/forms/labels/labeled-field.js

src/forms/inputs/checkbox-group.js

src/forms/helpers/dropdown-select.js

danparnella

I'll try to do one more holistic review, but I had a few comments on the changes since my last review.

danparnella · 2024-01-05T19:48:38Z

src/forms/labels/labeled-field.js

@@ -7,9 +7,9 @@ import { hasInputError } from '../helpers'

 /**
 *
- * A fieldset wrapper for redux-form controlled inputs. This wrapper adds a label component (defaults to {@link InputLabel})
+ * A container wrapper for redux-form controlled inputs. This wrapper adds a label component (defaults to {@link InputLabel})


I see we went with container, but then the rest of the description and the definition for the as param refer to the wrapper. So, might be best to stick with just wrapper?

src/forms/inputs/radio-group.js

migration-guides/v.10.0.0.md

danparnella · 2024-01-05T20:47:42Z

src/forms/labels/labeled-field.js

@@ -72,10 +73,12 @@ const propTypes = {
  ...InputError.propTypes,
  children: PropTypes.node,
  hideErrorLabel: PropTypes.bool,
+  as: PropTypes.string,


That works for me. I can't think of another element this would need to be off the top of my head.

test/forms/inputs/checkbox-group.test.js

test/forms/inputs/radio-group.test.js

src/forms/inputs/checkbox-group.js

test/forms/inputs/checkbox-group.test.js

danparnella

I found a few storybook accessibility violations for the components we've touched that we should be able to resolve pretty quickly.

src/forms/inputs/checkbox-group.js

src/forms/labels/labeled-field.js

chawes13 · 2024-01-15T15:37:21Z

Removed my review to take it out of my queue, please retag me when y'all are comfortable with the PR and ready for me to take a look!

chawes13 · 2024-02-09T15:41:38Z

@danparnella I'll wait until you give this a first pass. Please bump me when you're ready for another set of eyes!

danparnella

Updates resolved everything I saw previously. LGTM! Just need to resolve the version number conflict.

@chawes13 Ready for you to double-check!

chawes13

Lovely 👏

Confirmed test coverage remains at 100%! 🚀

src/forms/helpers/dropdown-select.js

…ab/lp-components into labeled-field-updates

jhp0621 added 13 commits August 1, 2023 12:14

Allows LabeledField to accept optional as prop and updates Checkbox+R…

d868e51

…adio Group

Uses css to visually hide label if set to false for Checkbox+Radio Gr…

3fa079b

…oups to preserve html semantics

Displays required indicator and hint if provided

49debb5

Updates dropdown-checkbox-group to match the same semantics as regula…

9873164

…r checkbox-group

Merge branch 'main' of https://github.com/LaunchPadLab/lp-components …

2dc1661

…into labeled-field-updates

Remove dropdown-checkbox-group component

f71254b

Add Dropdown in storybook

950cd7d

Remove unused css files

4d1c06c

Update styling

036cf6d

Define checkbox group container outside of component

4fbc017

Remove test suite re: missing legend

8c73fc3

Set group role on labeled field and simplify classname logic

c2df5fe

Move dropdown test suite into checkbox-group test file

04015e1

jhp0621 commented Sep 25, 2023

View reviewed changes

src/forms/inputs/checkbox-group.js Show resolved Hide resolved

jhp0621 commented Sep 25, 2023

View reviewed changes

src/forms/labels/labeled-field.js Outdated Show resolved Hide resolved

Merge branch 'main' into labeled-field-updates

e462dbb

jhp0621 marked this pull request as ready for review September 25, 2023 16:50

jhp0621 requested review from danparnella and chawes13 September 25, 2023 16:50

jhp0621 added 4 commits September 25, 2023 12:07

Update comment

4a7ee8c

Update version

4be9338

Update helper comment

62fd746

Undo defining variable outside

23ffe8a

danparnella reviewed Sep 25, 2023

View reviewed changes

src/forms/inputs/checkbox-group.js Outdated Show resolved Hide resolved

src/forms/inputs/dropdown-checkbox-group.js Outdated Show resolved Hide resolved

.storybook/styles/components/_forms.scss Outdated Show resolved Hide resolved

use prepare for yarn run build

ed0cf8e

danparnella requested changes Sep 27, 2023

View reviewed changes

chawes13 reviewed Sep 27, 2023

View reviewed changes

danparnella mentioned this pull request Sep 27, 2023

Input Label Accessibility Updates #514

Closed

5 tasks

danparnella requested changes Sep 27, 2023

View reviewed changes

update comments

c8352b9

jhp0621 removed the request for review from chawes13 November 6, 2023 19:52

use markup for components

952d3dd

jhp0621 requested review from chawes13 and danparnella November 6, 2023 19:55

Ji Hye Park and others added 2 commits November 13, 2023 17:00

pass in is-active conditional classname on higher level

efa4fc7

update dropdown select so the labels populate

d587379

jhp0621 commented Nov 14, 2023

View reviewed changes

src/forms/inputs/checkbox-group.js Show resolved Hide resolved

jhp0621 commented Nov 14, 2023

View reviewed changes

src/forms/helpers/dropdown-select.js Show resolved Hide resolved

danparnella mentioned this pull request Jan 5, 2024

Pull out legend into separate component #631

Open

danparnella requested changes Jan 5, 2024

View reviewed changes

src/forms/inputs/checkbox-group.js Show resolved Hide resolved

src/forms/labels/labeled-field.js Show resolved Hide resolved

danparnella mentioned this pull request Jan 12, 2024

Accessibility issues with LabeledField Stories #632

Closed

chawes13 removed their request for review January 15, 2024 15:36

jhp0621 added 5 commits February 8, 2024 13:31

Add a section about dropdown selection changes in migration guide

f6da097

Update comments

2f4dc31

wrap tests under describe

bcb4729

Use the correct labels for dropdown test

9814f6b

Pass in id for checkbox label that matches input id

6b9c6cf

jhp0621 requested review from danparnella and chawes13 February 8, 2024 21:56

danparnella approved these changes Feb 9, 2024

View reviewed changes

Merge branch 'main' into labeled-field-updates

c687232

chawes13 approved these changes Feb 12, 2024

View reviewed changes

src/forms/helpers/dropdown-select.js Outdated Show resolved Hide resolved

jhp0621 added 2 commits February 12, 2024 14:13

Use labels for safe guard check

c0dd1e7

Merge branch 'labeled-field-updates' of https://github.com/LaunchPadL…

5fb9bb7

…ab/lp-components into labeled-field-updates

jhp0621 merged commit 648a731 into main Feb 13, 2024
2 checks passed

jhp0621 deleted the labeled-field-updates branch February 13, 2024 02:08

danparnella mentioned this pull request Sep 9, 2024

aria-labelled by for LabeledField #436

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Labeled field updates #595

Labeled field updates #595

jhp0621 commented Aug 1, 2023 •

edited

Loading

danparnella left a comment

danparnella left a comment

chawes13 left a comment

danparnella left a comment

danparnella left a comment

danparnella Jan 5, 2024

danparnella Jan 5, 2024

danparnella left a comment

chawes13 commented Jan 15, 2024

chawes13 commented Feb 9, 2024

danparnella left a comment

chawes13 left a comment

Labeled field updates #595

Labeled field updates #595

Conversation

jhp0621 commented Aug 1, 2023 • edited Loading

Release Notes

Author Checklist

danparnella left a comment

Choose a reason for hiding this comment

danparnella left a comment

Choose a reason for hiding this comment

chawes13 left a comment

Choose a reason for hiding this comment

danparnella left a comment

Choose a reason for hiding this comment

danparnella left a comment

Choose a reason for hiding this comment

danparnella Jan 5, 2024

Choose a reason for hiding this comment

danparnella Jan 5, 2024

Choose a reason for hiding this comment

danparnella left a comment

Choose a reason for hiding this comment

chawes13 commented Jan 15, 2024

chawes13 commented Feb 9, 2024

danparnella left a comment

Choose a reason for hiding this comment

chawes13 left a comment

Choose a reason for hiding this comment

jhp0621 commented Aug 1, 2023 •

edited

Loading