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

feature(material-experimental/chips) Add grid keyboard shortcuts #16384

Merged
merged 1 commit into from
Jun 28, 2019

Conversation

vanessanschmitt
Copy link
Collaborator

No description provided.

@googlebot googlebot added the cla: yes PR author has agreed to Google's Contributor License Agreement label Jun 25, 2019
@vanessanschmitt
Copy link
Collaborator Author

@jelbourn

@vanessanschmitt
Copy link
Collaborator Author

related to #16173

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.

Here's my initial pass; I think I still have some details to look at more closely

src/material-experimental/mdc-chips/grid-key-manager.ts Outdated Show resolved Hide resolved
src/material-experimental/mdc-chips/chip-grid.ts Outdated Show resolved Hide resolved
* Example:
*
* `<mat-chip>
* <mat-icon matChipRemove>cancel</mat-icon>
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed this example (copied from the old implementation) is wrong. The element should be a button.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The mat-icon element should be a button? This example matches the demo, which seems to work well: https://github.com/angular/components/blob/master/src/dev-app/chips/chips-demo.html#L47

I did put 'role': 'button' on the host element for matChipRemove:
https://github.com/angular/components/pull/16384/files#diff-350c9b188f681cf2a48614ce9b6a4cffR100

Copy link
Member

Choose a reason for hiding this comment

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

Adding the role also works, it's just a style thing. We typically tell people to do

<button mat-icon-button aria-label="...">
  <mat-icon>...</mat-icon>
</button>

rather than putting click handlers on icons directly; it's mostly about setting a good example

Copy link
Collaborator Author

@vanessanschmitt vanessanschmitt Jun 28, 2019

Choose a reason for hiding this comment

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

That makes sense! The mdc classes right now don't play nicely with mat-icon-button. Can I fix this in a follow up PR? @jelbourn

src/material-experimental/mdc-chips/chip-icons.ts Outdated Show resolved Hide resolved
src/material-experimental/mdc-chips/chip-icons.ts Outdated Show resolved Hide resolved
src/material-experimental/mdc-chips/grid-key-manager.ts Outdated Show resolved Hide resolved
@jelbourn jelbourn requested a review from crisbeto June 25, 2019 23:41
@vanessanschmitt
Copy link
Collaborator Author

Style question: What are the rules for prefixing private/internal variables and methods with _?

@vanessanschmitt
Copy link
Collaborator Author

@jelbourn Just noting that I addressed the requested changes!

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.

Overall looks good, just one or two last nits

@jelbourn
Copy link
Member

We use an underscore for any non-public member, which includes both real private and the pretend "internal" members we have. Ideally we'd have to real way to mark things as "only usable within this package", but no such construct exists.

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 target: minor This PR is targeted for the next minor release labels Jun 28, 2019
@jelbourn jelbourn merged commit d29df38 into angular:master Jun 28, 2019
@vanessanschmitt vanessanschmitt deleted the devel branch July 1, 2019 15:51
@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 11, 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 target: minor This PR is targeted for the next minor release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants