Skip to content
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

feat: add simplified checkbox component for usage in other components #2619

Merged
merged 5 commits into from
Jan 27, 2017

Conversation

crisbeto
Copy link
Member

Adds the md-pseudo-checkbox, which is a simplified version of md-checkbox, that doesn't have the expensive SVG animations or the form control logic. This will be useful for multiple selection in md-select, as well as other components in the future.

Relates to #2412.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jan 12, 2017
Adds the `md-pseudo-checkbox`, which is a simplified version of `md-checkbox`, that doesn't have the expensive SVG animations or the form control logic. This will be useful for multiple selection in `md-select`, as well as other components in the future.

Relates to angular#2412.
@@ -132,7 +136,8 @@ export {NoConflictStyleCompatibilityMode} from './compatibility/no-conflict-mode
PortalModule,
OverlayModule,
A11yModule,
MdOptionModule
MdOptionModule,
MdSelectionModule
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing commas 4life

border-color $md-checkbox-transition-duration $md-linear-out-slow-in-timing-function,
background-color $md-checkbox-transition-duration $md-linear-out-slow-in-timing-function;

&::after {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add a comment explaining what the ::after element is used for?

cursor: default;
}

.md-pseudo-checkbox-indeterminate::after {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that the pseudo-checkbox needs an indeterminate state; did you have something in mind for it?

Also same question for disabled

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We definitely need the disabled state since select options can be disabled. Regarding the indeterminate state, we don't need it for the select, but I'd imagine that it would come in handy for other components (e.g. the "select all" functionality in the data table or potentially in the selection list).

@@ -0,0 +1,10 @@
@import './variables';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

File should be _checkbox-common.scss to be consistent with other shared styles

export type MdPseudoCheckboxState = 'unchecked' | 'checked' | 'indeterminate';

/**
* Simplified checkbox without any of the underlying form control logic
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about...

Component that shows a simplified checkbox without including any kind of "real" checkbox. 
Meant to be used when the checkbox is purely decorative and a large number of them will be 
included, such as for the options in a multi-select. Uses no SVGs or complex animations.

Note that this component will be completely invisible to screen-reader users. This is *not* 
interchangeable with <md-checkbox> and should not be used if the user would directly interact
 with the checkbox. The pseudo-checkbox should only be used as an implementation detail of 
more complex components that appropriately handle selected / checked state.

@Input() disabled: boolean = false;

/** Color of the checkbox. */
@Input()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of doing an @Input for color and disabled, perhaps we should just include the class directly? My reasoning is that it more clearly communicates that it's not a real checkbox and is only for show.

@kara thoughts?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that it's mainly for consistency. Regarding communication, this is intended for internal usage anyway, it shouldn't be an issue if people look at the readme.

@crisbeto
Copy link
Member Author

Addressed the feedback @jelbourn.

Copy link
Member

@jelbourn jelbourn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Jan 20, 2017
@andrewseguin andrewseguin merged commit 3b6cab0 into angular:master Jan 27, 2017
@angular-automatic-lock-bot
Copy link

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

This action has been performed automatically by a bot.

@angular-automatic-lock-bot angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 6, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
action: merge The PR is ready for merge by the caretaker cla: yes PR author has agreed to Google's Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants