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

Conversation

psamim
Copy link
Contributor

@psamim psamim commented Feb 2, 2020

PR Checklist

Please check that your PR fulfills the following requirements:

  • The commit message follows our guidelines
  • Tests for the changes have been added or this is not a bug fix / enhancement
  • Docs have been added, updated, or were not required

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Enhancement
[ ] Documentation content changes
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Infrastructure changes
[ ] Other... Please describe:

Issue Number:
#11854

Does this PR introduce a breaking change?

[ ] Yes
[x] No

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added the cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ label Feb 2, 2020
@psamim
Copy link
Contributor Author

psamim commented Feb 2, 2020

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@googlebot googlebot added cla: yes PR author has signed Google's CLA: https://opensource.google.com/docs/cla/ and removed cla: no PR author needs to sign Google's CLA: https://opensource.google.com/docs/cla/ labels Feb 2, 2020
@psamim psamim force-pushed the psamim-select-sanitize branch 3 times, most recently from 8ab8ae3 to c2651e8 Compare February 2, 2020 07:41
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.

Thank you for making your first PR to AngularJS Material! 💛

As mentioned below, I'd like to avoid adding duplicate code. So I'd like to add this capability to $mdUtil and then both components that need it can make use of the same function.

  • Please add a blank line and then "Fixes #11854" to your commit message. It's best to amend your commit and force push your branch after your updates are completed.
  • Please add a basic unit test for this. It should throw the same error prior to this fix and then work as expected with this fix in place.

@Splaktar Splaktar self-assigned this Feb 13, 2020
@Splaktar Splaktar added for: external contributor P3: important Important issues that really should be fixed when possible. type: bug labels Feb 13, 2020
@Splaktar Splaktar added this to the 1.1.22 milestone Feb 13, 2020
@Splaktar Splaktar added in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs needs: manual testing This issue or PR needs to have some manual testing and verification done needs: unit tests This PR needs unit tests to cover the changes being proposed labels Feb 13, 2020
@Splaktar Splaktar linked an issue Feb 13, 2020 that may be closed by this pull request
@psamim psamim force-pushed the psamim-select-sanitize branch from c2651e8 to 98c0424 Compare February 16, 2020 07:00
@psamim psamim requested a review from Splaktar February 16, 2020 07:15
@psamim
Copy link
Contributor Author

psamim commented Feb 16, 2020

@Splaktar I applied the requested changes. Thanks for the review.

@psamim psamim force-pushed the psamim-select-sanitize branch from 98c0424 to 673aace Compare February 16, 2020 07:35
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.

Thank you for making the requested updates. LGTM other than it needing to be rebased.

@Splaktar
Copy link
Contributor

Splaktar commented Mar 6, 2020

✅ Rebased.

@Splaktar Splaktar removed needs: unit tests This PR needs unit tests to cover the changes being proposed in progress Mainly for in progress PRs, but may be used for issues that require multiple PRs labels Mar 6, 2020
@Splaktar Splaktar requested a review from mmalerba March 6, 2020 02:41
Copy link
Contributor

@mmalerba mmalerba 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 and removed needs: manual testing This issue or PR needs to have some manual testing and verification done labels Mar 6, 2020
@Splaktar Splaktar merged commit 0ec0cc5 into angular:master Mar 6, 2020
@Splaktar Splaktar added pr: lgtm This PR has been approved by the reviewer and removed for: external contributor labels May 15, 2020
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/ P3: important Important issues that really should be fixed when possible. pr: lgtm This PR has been approved by the reviewer pr: merge ready This PR is ready for a caretaker to review type: bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

select: SyntaxError: Invalid regular expression: nothing to repeat
4 participants