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(chips): Add (select), [color] and dark theme to chips. #2242

Merged
merged 1 commit into from
Dec 20, 2016

Conversation

topherfangio
Copy link
Contributor

Add new functionality/options to chips for managing selection/color.

MdChipList Options

  • [selectable] - Programmatically control whether or not the chips
    in the list are capable of being selected.
  • The SPACE key will automatically select the currently focused chip.

MdChip Options

  • [color] - Programmatically control the selected color of the
    chip.
  • [selected] - Programmatically control whether or not the chip
    is selected.
  • (select) - Event emitted when the chip is selected.
  • (deselect) - Event emitted when the chip is deselected.

Additionally, adds basic support for dark themeed chips using existing
colors from other components in the spec and cleanup demos by using
cards and toolbars like other demos.

References #120.


screen shot 2016-12-15 at 2 39 01 pm

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Dec 15, 2016
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.

I pulled down this PR to try it out and realized that the chip keys are using up/down instead of left/right. Is that something you planned on changing?

* Whether or not this chip is selectable. If false, `selected` *cannot* be set to `true`.
*
* Note: Disables keyboard *and* programmatic interaction.
*/
Copy link
Member

Choose a reason for hiding this comment

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

Can we instead make this

/**
 * Whether or not this chip is selectable. When a chip is not selectable, 
 * its `selected` state is always ignored. 
 */

}
}

toggleSelectOnFocusedChip(): void {
Copy link
Member

Choose a reason for hiding this comment

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

Should this method me internal (_toggleSelectOnFocusedChip)?

this._selectable = coerceBooleanProperty(value);
}

/** Pass relevant focus events to our key manager. */
Copy link
Member

Choose a reason for hiding this comment

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

This method needs a more public-facing description.

return this.selected;
}

/** Returns the current color of this chip. */
Copy link
Member

Choose a reason for hiding this comment

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

Use:

/** The color of the chip. Can be `primary`, `accent`, or `warn`. */

Omit the setter comment (the getter/setter pair is treated as a single property for docs)

return this._selected;
}

/** Programatically sets the selected state of this chip. */
Copy link
Member

Choose a reason for hiding this comment

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

Omit setter comment

@@ -92,4 +137,27 @@ export class MdChip implements MdFocusable, OnInit, OnDestroy {
this.focus();
}
}

/** Initializes the appropriate CSS classes based on the chip type (basic or standard). */
private _initClasses() {
Copy link
Member

Choose a reason for hiding this comment

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

_addDefaultCssClass?

@@ -26,6 +26,7 @@ export interface MdChipEvent {
'tabindex': '-1',
'role': 'option',

'[class.selected]': 'selected',
Copy link
Member

Choose a reason for hiding this comment

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

md-chip-selected

$foreground: map-get($theme, foreground);

$unselected-background: if($is-dark-theme, md-color($background, card), #e0e0e0);
$unselected-foreground: if($is-dark-theme, md-color($foreground, text), rgba(0, 0, 0, 0.87));
Copy link
Member

Choose a reason for hiding this comment

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

Why are these not just

$unselected-background: md-color($background, card);
$unselected-foreground: md-color($foreground, text);
// etc.

?

The background and foreground colors are already based on whether it's a light/dark theme. Is there some reason the light theme version of these colors aren't appropriate?

@topherfangio
Copy link
Contributor Author

@jelbourn Requested changes made.

Regarding the left/right arrow keys: I plan to add that at a later date, but many screen readers have difficulty with left/right which can cause problems (and did so with the md1 version). I'd like to do some testing before we add it so that I can work around any problems.

Add new functionality/options to chips for managing selection/color.

*MdChipList Options*

 - `[selectable]` - Programmatically control whether or not the chips
   in the list are capable of being selected.
 - The SPACE key will automatically select the currently focused chip.

*MdChip Options*

 - `[color]` - Programmatically control the selected color of the
   chip.
 - `[selected]` - Programmatically control whether or not the chip
   is selected.
 - `(select)` - Event emitted when the chip is selected.
 - `(deselect)` - Event emitted when the chip is deselected.

Additionally, adds basic support for dark themeed chips using existing
colors from other components in the spec and cleanup demos by using
cards and toolbars like other demos.

References angular#120.
@topherfangio
Copy link
Contributor Author

FYI - I introduced a small bug in my last commit, so I added a new test when I fixed it. Just checks for proper CSS classes when the chip is selected.

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

We definitely need to make left/right work, since it would be even more confusing for sighted users.

@jelbourn jelbourn added pr: lgtm action: merge The PR is ready for merge by the caretaker and removed pr: needs review labels Dec 18, 2016
@jelbourn jelbourn merged commit f45c315 into angular:master Dec 20, 2016
@topherfangio topherfangio deleted the team/topher/chips-select-120 branch January 4, 2017 18:15
@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.

3 participants