Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

Conversation

Ignigena
Copy link
Contributor

Add basic support for md-separator-keys. The ENTER and TAB keys trigger
separation by default through md-autocomplete regardless of what
additional key codes are user defined.

Implements #6097, continuation of #6127 with the addition of unit tests requested.

@Frank3K
Copy link
Contributor

Frank3K commented Jun 2, 2016

Why was this closed? The previous PR (#6127) was skipped due to lacking unit tests. This PR includes tests and was ready to pull as far as I can see.

@ThomasBurleson
Copy link
Contributor

This issue is closed as part of our ‘Surge Focus on Material 2' efforts.
For details, see our forum posting @ http://bit.ly/1UhZyWs.

@Splaktar Splaktar modified the milestones: Deprecated, 1.1.10 Apr 25, 2018
@Splaktar Splaktar added type: feature P4: minor Minor issues. May not be fixed without community contributions. needs: review This PR is waiting on review from the team labels Apr 25, 2018
@Splaktar Splaktar self-requested a review April 25, 2018 02:48
@Splaktar Splaktar reopened this Apr 25, 2018
@Splaktar Splaktar added the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Apr 25, 2018
@Splaktar Splaktar modified the milestones: 1.1.10, 1.1.11 Jun 19, 2018
@Splaktar Splaktar self-assigned this Jun 19, 2018
@Splaktar
Copy link
Contributor

@Ignigena can you please rebase this? I would like to get it into 1.1.11 if possible. Thank you for your contribution!

Add basic support for md-separator-keys. The ENTER and TAB keys trigger
separation by default through md-autocomplete regardless of what
additional key codes are user defined.
@Ignigena Ignigena force-pushed the feature/contact-separator branch from 1562023 to 033fd96 Compare June 19, 2018 15:48
@googlebot googlebot added the cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ label Jun 19, 2018
@Ignigena
Copy link
Contributor Author

@Splaktar wow, didn't expect this to ever get pushed through. I've rebased and should be more or less good to go. Let me know if anything needs to be adjusted since I haven't used Angular or Material in production since around the time of the "surge focus" that originally closed this.

@Splaktar Splaktar removed the needs: rebase This PR needs to be rebased on the latest commits from master and conflicts need to be resolved label Jun 19, 2018
@Splaktar
Copy link
Contributor

Thank you very much! I'll try to get the review done soon. Sorry for the many delays in getting this merged :)

@Ignigena
Copy link
Contributor Author

@Splaktar you are doing fantastic work on this project!

@Splaktar Splaktar added needs: manual testing This issue or PR needs to have some manual testing and verification done and removed needs: review This PR is waiting on review from the team labels Jun 20, 2018
Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

Initial review looks good, but it raised a few questions about differences between this and the md-chips implementation that I want to verify with some manual testing.

Copy link
Contributor

@Splaktar Splaktar left a comment

Choose a reason for hiding this comment

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

LGTM

@Splaktar Splaktar added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed needs: manual testing This issue or PR needs to have some manual testing and verification done labels Jul 8, 2018
@Splaktar
Copy link
Contributor

Splaktar commented Jul 8, 2018

Manual testing went well. I found some issues with existing documentation that I will open a separate PR for.

Splaktar added a commit that referenced this pull request Jul 8, 2018
using md-highlight-text on contact chips actually breaks the component
only md-highlight-flags is supported on contact chips
add md-highlight-flags example to mdHighlightDirective docs

Relates to #8142
jelbourn pushed a commit that referenced this pull request Jul 11, 2018
using md-highlight-text on contact chips actually breaks the component
only md-highlight-flags is supported on contact chips
add md-highlight-flags example to mdHighlightDirective docs

Relates to #8142
}

event.stopPropagation();
event.preventDefault();
Copy link
Member

Choose a reason for hiding this comment

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

A component should only ever use preventDefault or stopPropagation when when it actually handles the specific key being pressed; this looks like it's doing it for every keypress into the chips component. This would result in blocking standard browser behaviors that people depend on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jelbourn This should only be called if the specific keypress is mapped to a key code in the separatorKeys configuration. Line 22 will immediately return if it's not a matching key so this should not run on every keypress.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I misread that condition.

@jelbourn jelbourn added pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer and removed pr: merge ready This PR is ready for a caretaker to review pr: lgtm This PR has been approved by the reviewer labels Jul 11, 2018
@jelbourn jelbourn merged commit eb10b56 into angular:master Jul 11, 2018
Splaktar added a commit that referenced this pull request Jul 31, 2018
using md-highlight-text on contact chips actually breaks the component
only md-highlight-flags is supported on contact chips
add md-highlight-flags example to mdHighlightDirective docs

Relates to #8142
Splaktar pushed a commit that referenced this pull request Jul 31, 2018
Add basic support for md-separator-keys. The ENTER and TAB keys trigger
separation by default through md-autocomplete regardless of what
additional key codes are user defined.
Splaktar added a commit that referenced this pull request Aug 2, 2018
using md-highlight-text on contact chips actually breaks the component
only md-highlight-flags is supported on contact chips
add md-highlight-flags example to mdHighlightDirective docs

Relates to #8142
Splaktar pushed a commit that referenced this pull request Aug 2, 2018
Add basic support for md-separator-keys. The ENTER and TAB keys trigger
separation by default through md-autocomplete regardless of what
additional key codes are user defined.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ P4: minor Minor issues. May not be fixed without community contributions. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants