-
Notifications
You must be signed in to change notification settings - Fork 7
Remove Form Controls Layout #1199
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: 8723339 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
🚀 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