New Shoelace release 2.1.0 #35
Conversation
… implementations won't break
…updated to Font Awesome
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
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)
| 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; | ||
| }); |
There was a problem hiding this comment.
Is this different than the check on line 12?
There was a problem hiding this comment.
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
| `); | ||
| const checkbox = checkboxGroup.querySelector<SlCheckbox>('#checkbox-1')!; | ||
| setTimeout(() => checkbox.click()); | ||
| const event = (await oneEvent(checkboxGroup, 'sl-change')) as SlChangeEvent; |
There was a problem hiding this comment.
I'm missing where we're testing sl-input in this one
There was a problem hiding this comment.
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!
| const anyCheckboxChecked = this.value.some(value => value.includes('true')); | ||
| const isRequiredAndEmpty = this.required && !anyCheckboxChecked; | ||
| const hasCustomValidityMessage = this.customValidityMessage !== ''; |
There was a problem hiding this comment.
These are duplicated at least once below -- would they be better as helper methods rather than locals?
There was a problem hiding this comment.
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
| if (this.value === null || this.value === undefined) { | ||
| this.initializeValueFromCheckboxes(); | ||
| } |
There was a problem hiding this comment.
Just my unfamiliarity here -- do we need this still despite it being called from connectedCallback?
There was a problem hiding this comment.
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.
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 This addresses all your comments, I believe? Let me know what you think! |
What's in the PR (Highlights)
sl-switchto include new layout options (label left, label left justified), plus some minor styling updatessl-radioandsl-radio-groupcomponents:label-tooltipandcontext-notehorizontallayout optioncontainedoption is applied, so that it can be set just once fromsl-radio-groupand cascade to childsl-radioelementssl-checkbox-groupcomponent, based on the existingsl-radio-groupcomponent:horizontallayout option to the Checkbox Groupcontainedoption can be applied to the Checkbox Group, to match the way this option works for the Radio Groupsl-dialogsl-alert, plus change defaultsl-toast-stackposition from top right to bottom rightcheckedandx-logsystem icons to use Font Awesome svgssl-checkbox-groupcomponent, 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 existingsl-radio-groupas a model, but the behavior of thesl-checkbox-groupis 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 thesl-radio-grouppattern exactly.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 tonext