New Shoelace release 2.1.0 #35

slhinyc · 2024-04-29T23:12:29Z

What's in the PR (Highlights)

Updated sl-switch to include new layout options (label left, label left justified), plus some minor styling updates
Updated sl-radio and sl-radio-group components:
- Added form component options:label-tooltip and context-note
- Added horizontal layout option
- Updated how contained option is applied, so that it can be set just once from sl-radio-group and cascade to child sl-radio elements
- Minor styling updates
Added new sl-checkbox-group component, based on the existing sl-radio-group component:
- So that a group of checkboxes can have a label, help text, label tooltip, and context note, just like other form components
- Added a horizontal layout option to the Checkbox Group
- contained option can be applied to the Checkbox Group, to match the way this option works for the Radio Group
Small example updates and styling fixes to sl-dialog
Small example updates to sl-alert, plus change default sl-toast-stack position from top right to bottom right
Update checked and x-log system icons to use Font Awesome svgs

👉 Also see the Changelog page in the review app for a full list of changes for this release, which also includes changes from a previous PR that have already been merged to next but not yet tagged for release (we were holding off on cutting a new release until all the form-related components updates had been merged)

‼️ Note to reviewers:

Please review the new sl-checkbox-group component, including the tests and validity checks, closely. Although everything is working as expected, I'm not confident that I have done everything correctly here. I used the existing sl-radio-group as a model, but the behavior of the sl-checkbox-group is a bit different (multiple items can be checked; the group is valid if at least one checkbox in the group is checked), so I couldn't follow the sl-radio-group pattern exactly.
There are a lot of other changes here, but most of them are styling and doc site example updates, so I'm less worried about those.
There are also some new form component properties here -- the label-tooltip, context-note -- but these are following patterns already applied to other components & reviewed and approved by Daross in a previous PR that's already been merged to next

…xamples

… implementations won't break

…rapper

…kboxes

…updated to Font Awesome

vercel · 2024-04-29T23:12:36Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
shoelace	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	May 6, 2024 8:05pm

kdonovan

Thanks for doing this, @slhinyc! Giant caveat that I'm super unfamiliar with all of this, but I got it spun up locally and everything looks generally good!

One likely issue: when I submit the required checkbox group without selecting any, the submit event still fires -- and then the browser-default validation error appears (sidenote: I don't know if it's under our control, but that message is a little confusing since it's actually submit any of those boxes).

(I thought we had a test for this, but on second look I think we're only testing that submit isn't fired if we've already set setCustomValidity)

kdonovan · 2024-05-01T23:24:30Z

src/components/checkbox-group/checkbox-group.test.ts

+    it(`should be invalid when required and no checkboxes are checked`, async () => {
+      const el = await fixture<SlCheckboxGroup>(html`
+        <sl-checkbox-group label="Select an option" required>
+          <sl-checkbox name="option" value="1">Option 1</sl-checkbox>
+          <sl-checkbox name="option" value="2">Option 2</sl-checkbox>
+          <sl-checkbox name="option" value="3">Option 3</sl-checkbox>
+        </sl-checkbox-group>
+      `);
+
+      expect(el.checkValidity()).to.be.false;
+    });


Is this different than the check on line 12?

I see what you mean. I was trying to follow the test patterns for the radio-group for these (these are the un-edited tests from the version of Shoelace we're using), and the radio-group.test.ts essentially has the same thing:

If you compare the tests at line 13 vs line 50 in this file: src/components/radio-group/radio-group.test.ts

kdonovan · 2024-05-01T23:29:43Z

src/components/checkbox-group/checkbox-group.test.ts

+    `);
+    const checkbox = checkboxGroup.querySelector<SlCheckbox>('#checkbox-1')!;
+    setTimeout(() => checkbox.click());
+    const event = (await oneEvent(checkboxGroup, 'sl-change')) as SlChangeEvent;


I'm missing where we're testing sl-input in this one

This is another one where I was following the pattern in the same test for src/components/radio-group/radio-group.test.ts, but maybe it's a mistake in the Radio Group test? I just updated both to include an event listener for sl-input. Pushed a new commit!

kdonovan · 2024-05-01T23:31:23Z

src/components/checkbox-group/checkbox-group.component.ts

+    const anyCheckboxChecked = this.value.some(value => value.includes('true'));
+    const isRequiredAndEmpty = this.required && !anyCheckboxChecked;
+    const hasCustomValidityMessage = this.customValidityMessage !== '';


These are duplicated at least once below -- would they be better as helper methods rather than locals?

This is another instance where I was following what's in the Radio Group component (the Shoelace default code), where some locals are repeated. Is it okay to leave, to keep things consistent across components?

See lines 103-104 and lines 117-118 in this file: src/components/radio-group/radio-group.test.ts

kdonovan · 2024-05-01T23:32:56Z

src/components/checkbox-group/checkbox-group.component.ts

+    if (this.value === null || this.value === undefined) {
+      this.initializeValueFromCheckboxes();
+    }


Just my unfamiliarity here -- do we need this still despite it being called from connectedCallback?

I added this because while all the tests were passing, I was getting this TypeError (see SS), and adding this if statement made it go away! I tried to adjust the tests a bunch to figure out why this is happening, but couldn't figure it out. If it's okay that I have this TypeError as long as the tests themselves are passing, then I can just delete this from the main component file, as everything is working as expected without this being repeated in the validity methods.

…put`

slhinyc · 2024-05-06T20:14:48Z

One likely issue: when I submit the required checkbox group without selecting any, the submit event still fires -- and then the browser-default validation error appears (sidenote: I don't know if it's under our control, but that message is a little confusing since it's actually submit any of those boxes).

So this looks like an error in the example code, which I copied from the Radio Group example, but it was wrong there, too! I figured out what was missing. Following the examples on the Form Controls doc page, we needed an await Promise.all to "Wait for controls to be defined before attaching form listeners". This seems to fix the issue & the examples now all work as expected. I also fixed the issue on two Radio Group validation examples, and also added a "Set custom validity" example to the Checkbox Group, as I realized that I was missing that one.

This addresses all your comments, I believe? Let me know what you think!

kdonovan

checkboxgroup validation is working now -- LGTM! 🚗

slhinyc added 24 commits March 16, 2024 16:08

Update <sl-switch> properties, styles, doc site examples

2466507

Update <sl-radio> and <sl-radio-group> properties, styles, doc site e…

7d59feb

…xamples

Add <sl-checkbox-group> component; related updates to <sl-checkbox>

d41e2b2

Updates to Radio, Radio Group, related to new Checkbox Group component

4d1751f

Misc doc site example and guidelines updates

d0168de

Fix 'contained' checkbox group and radio group logic so that existing…

d77ee73

… implementations won't break

Update checkbox and radio styles and props to adjust for simpleform w…

cd4c37f

…rapper

Update Checkbox Group value to store checked state of all nested chec…

e438603

…kboxes

Fix token mapping for sl-spacing-large

9c8de06

Update dialog specing, icons, examples, and usage guidelines

fb9043e

Update danger text color token value from 800 to 700

c817783

Make sure Checkbox Group checks validity initially

5ce091a

Fix inconsistent body padding for small dialog with header icon

217b672

Add usage guidelines for sl-dialog

6e29081

Add new checkbox-group component

2cca9bb

Fix misc failing tests

13a8d86

Add comments to library.system.ts so we know which icons have been …

af0adc3

…updated to Font Awesome

Update alert toast stack position and examples

6b45689

Update dialog padding and font size for small dialog

68c9e65

Update changelog.md

b842510

More alert example updates

03f7a92

Update checkbox border color when checkbox has focus

8b7ecfb

Fix padding for "Dialog as Wrapper" example

f08d247

Hide required asterisk for individual checkboxes

2b7c573

slhinyc requested review from kdonovan, adrianbautista and a team April 29, 2024 23:12

slhinyc requested a review from davesims April 30, 2024 15:46

slhinyc requested a review from kathleenteamshares April 30, 2024 15:46

kdonovan requested a review from Workman May 1, 2024 22:57

kdonovan requested changes May 1, 2024

View reviewed changes

Add line-height to Tab style so it doesn't inherit from body

eae74cc

vercel bot deployed to Preview May 2, 2024 14:58 View deployment

Update checkbox-group & radio-group tests to fix references to `sl-in…

e6f99b7

…put`

vercel bot deployed to Preview May 3, 2024 20:01 View deployment

slhinyc added 4 commits May 6, 2024 11:00

Update test description in checkbox-group.test.ts to match test

884cd5c

Update to delete unnecessary initializeValue check in validity methods

05d3793

Update checkbox-group tests to better match radio groups

e143a47

Update checkbox group and radio group validity examples

f77a382

vercel bot deployed to Preview May 6, 2024 20:05 View deployment

slhinyc requested a review from kdonovan May 6, 2024 20:25

kdonovan approved these changes May 6, 2024

View reviewed changes

slhinyc merged commit 44ae748 into next May 6, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New Shoelace release 2.1.0 #35

New Shoelace release 2.1.0 #35

slhinyc commented Apr 29, 2024

vercel bot commented Apr 29, 2024 •

edited

Loading

kdonovan left a comment

kdonovan May 1, 2024

slhinyc May 3, 2024

kdonovan May 1, 2024

slhinyc May 6, 2024

kdonovan May 1, 2024

slhinyc May 3, 2024

kdonovan May 1, 2024

slhinyc May 6, 2024

slhinyc commented May 6, 2024 •

edited

Loading

kdonovan left a comment

New Shoelace release 2.1.0 #35

New Shoelace release 2.1.0 #35

Conversation

slhinyc commented Apr 29, 2024

What's in the PR (Highlights)

‼️ Note to reviewers:

vercel bot commented Apr 29, 2024 • edited Loading

kdonovan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

slhinyc commented May 6, 2024 • edited Loading

kdonovan left a comment

Choose a reason for hiding this comment

vercel bot commented Apr 29, 2024 •

edited

Loading

slhinyc commented May 6, 2024 •

edited

Loading