-
Notifications
You must be signed in to change notification settings - Fork 7
Remove Form Controls Layout #1199
Conversation
🦋 Changeset detectedLatest commit: 78c0fab The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
5b95902
to
12c8d84
Compare
readonly = false; | ||
|
||
@property({ attribute: 'select-all', reflect: true, type: Boolean }) | ||
selectAll = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alphabetized this and the other properties. No changes other than the addition of split
. Same for Slider.
12c8d84
to
95086ce
Compare
src/checkbox.ts
Outdated
} | ||
|
||
set split(split: 'left' | 'middle' | 'right' | undefined) { | ||
if (this.orientation === 'vertical') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this be overkill?
if (this.orientation === 'vertical') { | |
if (split && this.orientation === 'vertical') { |
In case somehow this split
setter validly hits with it being set to undefined
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not to me. That seems right given consumers can split and unsplit. I'll ignore this suggestion and push a commit changing it everywhere.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
8723339 ✅
0e1004d
to
19d479a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
He may also want to wait to merge until our other unreleased changes have been digested by consuming applications.
I'm good with merging this now as the migration path is pretty straight forward. Looks like most consumers don't override the default, if one is provided, so it'll be as simple as removing the wrapping component and setting split="left"
.
Rebased to resolve the conflict. |
8723339
to
78c0fab
Compare
🚀 Description
I'll wait to merge until @ynotdraw gets back. He'll be interested in reviewing. He may also want to wait to merge until our other unreleased changes have been digested by consuming applications.
📋 Checklist
🔬 Manual Testing