Radio and checkbox icon buttons #1892
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1892 +/- ##
========================================
Coverage ? 100%
========================================
Files ? 424
Lines ? 8968
Branches ? 1326
========================================
Hits ? 8968
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
|
Created #1907 to document new components. |
| @@ -25,4 +25,34 @@ export class SkyCheckboxDemoComponent { | |||
| disabled: true | |||
| } | |||
| ]; | |||
| public iconCheckboxItems = [ | |||
There was a problem hiding this comment.
I noticed none of these are disabled. Should we show at least one disabled value to show consumers how to do it?
There was a problem hiding this comment.
Yeah, that's a good plan. I'm going to add strikethrough to the group example.
src/scss/_forms.scss
Outdated
| @@ -118,3 +153,23 @@ fieldset { | |||
| flex: 1 1 auto; | |||
| width: 100%; | |||
| } | |||
|
|
|||
| .sky-switch-icon-group{ | |||
There was a problem hiding this comment.
Where is sky-switch-icon-group being used? I can't find it anywhere in our codebase.
There was a problem hiding this comment.
Sorry, I should have messaged you. Todd just added this as a class that people can put on the container of their icon buttons (radio or checkbox) to have them appear as an inline group. I'm adding an example to each demo now
There was a problem hiding this comment.
Ah cool! That will be helpful. I noticed there's some style inconsistencies with this block of CSS - space before the bracket and some tabs needed. Other than that, I'm good with this 👍
| 'sky-switch-control-danger': checkboxType === 'danger' | ||
| }"> | ||
| <sky-icon | ||
| aria-hidden="true" |
There was a problem hiding this comment.
I don't think you need aria-hidden anymore with the new icon component, do you?
There was a problem hiding this comment.
This is true. I do not need this anymore 👍
Removing.
| return this._checkboxType || 'info'; | ||
| } | ||
| public set checkboxType(value: string) { | ||
| this._checkboxType = value; |
There was a problem hiding this comment.
Do you want to also do a if(value) check and a .toLowerCase() here?
There was a problem hiding this comment.
Yes, I do want that. Added
| useExisting: forwardRef(() => SkyCheckboxIconComponent), | ||
| multi: true | ||
| }; | ||
|
|
There was a problem hiding this comment.
Should we have a // tslint:enable here?
There was a problem hiding this comment.
oh yeah, nice catch
| <span | ||
| class="sky-switch-control sky-rounded-corners"> | ||
| <sky-icon | ||
| *ngIf="checked" | ||
| aria-hidden="true" |
There was a problem hiding this comment.
Same comment as before - can you get rid of aria-hidden now?
There was a problem hiding this comment.
removed
| public radioType: string; | ||
| } | ||
|
|
||
| describe('Icon radio component', () => { |
There was a problem hiding this comment.
Should this say Radio icon component ? I only ask because if we're quickly scanning the list of tests, you might have trouble finding it since its not consistent with the component name.
There was a problem hiding this comment.
That's a good point. I'll switch them around
| public checkboxType: string; | ||
| } | ||
|
|
||
| describe('Icon checkbox component', () => { |
There was a problem hiding this comment.
Same comment as before - Checkbox icon component.
There was a problem hiding this comment.
switched
|
updated @Blackbaud-AlexKingman 👍 |
…yux2 into radio-checkbox-icon-buttons
|
Talked offline about not using rounded corners in the groups. Once thats done, I'm good to go with this! |
|
@Blackbaud-AlexKingman I've updated the demos to use the groupings properly. They were already set, the icon components have to be direct children of the switch-group element. |
|
Smooth. I like it 🚢 🇮🇹 ! |
Blackbaud-AlexKingman
added
Status: Ready to merge
and removed
Status: In progress
labels
Blackbaud-AlexKingman
assigned blackbaud-conorwright and unassigned Blackbaud-AlexKingman
|
On hold for: blackbaud/skyux-theme#23 |
blackbaud-conorwright
added
Status: Ready to merge
Status: Ready for review
and removed
Status: On hold
Status: Ready to merge
labels
blackbaud-conorwright
added
Status: Ready to merge
sky-checkbox
Severity: Medium
and removed
Status: Ready for review
labels
Created new radio and checkbox icon button components.
The tag names are up for debate, I'm still not sure if
sky-checkbox-iconandsky-radio-iconare clear enough. They seem like they could be confused with an icon component that can be placed inside of these elements.Resolves: #970
Original contribution: #1644