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

fix(router): make routerLinkActive work with query params arrays #22666

Closed
wants to merge 1 commit into from

Conversation

@Zaid-Al-Omari
Copy link
Contributor

Zaid-Al-Omari commented Mar 8, 2018

The url_tree equalQueryParams and containsQueryParam methods did not handle query params that has arrays, which resulted in the routerLinkActive to not behave as expected, change was made to ensure query params with arrays are handled correctly

fixes #22223

PR Type

What kind of change does this PR introduce?

[x] Bugfix
[ ] Feature
[ ] Code style update (formatting, local variables)
[ ] Refactoring (no functional changes, no api changes)
[ ] Build related changes
[ ] CI related changes
[ ] Documentation content changes
[ ] angular.io application / infrastructure changes
[ ] Other... Please describe:

What is the current behavior?

Issue Number: #22223

What is the new behavior?

Arrays in query params are now handled properly when using routerLinkActive

Does this PR introduce a breaking change?

[ ] Yes
[x] No

Other Information

Done by @Zaid-Al-Omari

@googlebot

This comment has been minimized.

Copy link

googlebot commented Mar 8, 2018

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 (e.g. I signed it!) and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers
  • Your company has a Point of Contact who decides which employees are authorized to participate. Ask your POC to be added to the group of authorized contributors. If you don't know who your Point of Contact is, direct the project maintainer to go/cla#troubleshoot.
  • The email used to register you as an authorized contributor must be the email used for the Git commit. Check your existing CLA data and verify that your email is set on your git commits.
  • The email used to register you as an authorized contributor must also be attached to your GitHub account.
@googlebot googlebot added the cla: no label Mar 8, 2018
@googlebot

This comment has been minimized.

Copy link

googlebot commented Mar 9, 2018

CLAs look good, thanks!

@googlebot googlebot added cla: yes and removed cla: no labels Mar 9, 2018
@googlebot

This comment has been minimized.

Copy link

googlebot commented Mar 9, 2018

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of the commit author(s) and merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Mar 9, 2018
@Zaid-Al-Omari Zaid-Al-Omari force-pushed the Zaid-Al-Omari:fix22223 branch from 7b5b775 to 32c07f8 Mar 9, 2018
@Zaid-Al-Omari

This comment has been minimized.

Copy link
Contributor Author

Zaid-Al-Omari commented Mar 9, 2018

I Have signed the CLA and the bot accepted it.
But then i made a merge by mistake, reset again and rebased (deleted the merge). So the other authors commits where removed from this pull request.

Please review the changes and let me know if anything needs to be changed.

@jasonaden

@Zaid-Al-Omari Zaid-Al-Omari force-pushed the Zaid-Al-Omari:fix22223 branch from 32c07f8 to 2274167 Mar 9, 2018
@kara kara added the comp: router label Mar 9, 2018
@Zaid-Al-Omari Zaid-Al-Omari force-pushed the Zaid-Al-Omari:fix22223 branch 3 times, most recently from 45bb20a to ab24a4f Mar 10, 2018
@jasonaden jasonaden added this to the needsTriage milestone Jan 29, 2019
@mhevery mhevery self-assigned this Nov 8, 2019
@IgorMinar IgorMinar modified the milestones: needsTriage, v9-candidates Nov 9, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 11, 2019

A Googler has manually verified that the CLAs look good.

(Googler, please make sure the reason for overriding the CLA status is clearly documented in these comments.)

ℹ️ Googlers: Go here for more info.

@ngbot

This comment has been minimized.

Copy link

ngbot bot commented Nov 11, 2019

I see that you just added the PR action: merge label, but the following checks are still failing:
    failure conflicts with base branch "master"
    pending status "code-review/pullapprove" is pending
    pending status "google3" is pending
    pending missing required status "ci/circleci: publish_snapshot"
    pending missing required status "ci/angular: size"

If you want your PR to be merged, it has to pass all the CI checks.

If you can't get the PR to a green state due to flakes or broken master, please try rebasing to master and/or restarting the CI job. If that fails and you believe that the issue is not due to your change, please contact the caretaker and ask for help.

@mhevery mhevery force-pushed the Zaid-Al-Omari:fix22223 branch from ab24a4f to 8c2ff0b Nov 11, 2019
@Zaid-Al-Omari Zaid-Al-Omari requested a review from angular/fw-router as a code owner Nov 11, 2019
@googlebot

This comment has been minimized.

Copy link

googlebot commented Nov 11, 2019

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

…in arrays

The url_tree equalQueryParams and containsQueryParam methods did not handle query params that has arrays, which resulted in the routerLinkActive to not behave as expected, change was made to ensure query params with arrays are handled correctly

fixes #22223
@mhevery mhevery force-pushed the Zaid-Al-Omari:fix22223 branch from 8c2ff0b to 4b2b24a Nov 11, 2019
@kara

This comment has been minimized.

Copy link
Contributor

kara commented Nov 12, 2019

@kara kara closed this in b30bb8d Nov 13, 2019
kara added a commit that referenced this pull request Nov 13, 2019
…in arrays (#22666)

The url_tree equalQueryParams and containsQueryParam methods did not handle query params that has arrays, which resulted in the routerLinkActive to not behave as expected, change was made to ensure query params with arrays are handled correctly

fixes #22223

PR Close #22666
alxhub added a commit to alxhub/angular that referenced this pull request Nov 15, 2019
alxhub added a commit to alxhub/angular that referenced this pull request Nov 15, 2019
…ch contain arrays (angular#22666)"

This reverts commit b30bb8d.

Reason: breaks internal g3 project.
alxhub added a commit that referenced this pull request Nov 15, 2019
…ch contain arrays (#22666)" (#33861)

This reverts commit b30bb8d.

Reason: breaks internal g3 project.

PR Close #33861
alxhub added a commit that referenced this pull request Nov 15, 2019
…ch contain arrays (#22666)" (#33861)

This reverts commit b30bb8d.

Reason: breaks internal g3 project.

PR Close #33861
@alxhub alxhub reopened this Nov 15, 2019
@alxhub

This comment has been minimized.

Copy link
Contributor

alxhub commented Nov 15, 2019

Hi @Zaid-Al-Omari,

Apologies, but this PR had to be reverted as it broke a test in an internal Google project. Details of the breakage here available here (for Googlers only).

ajitsinghkaler added a commit to ajitsinghkaler/angular that referenced this pull request Nov 18, 2019
…in arrays (angular#22666)

The url_tree equalQueryParams and containsQueryParam methods did not handle query params that has arrays, which resulted in the routerLinkActive to not behave as expected, change was made to ensure query params with arrays are handled correctly

fixes angular#22223

PR Close angular#22666
@agale123

This comment has been minimized.

Copy link
Contributor

agale123 commented Nov 20, 2019

We found the bug that caused a test to fail within Google and it has been fixed. So presubmits can be run again, and should pass this time

matsko added a commit that referenced this pull request Nov 22, 2019
…in arrays (#22666)

The url_tree equalQueryParams and containsQueryParam methods did not handle query params that has arrays, which resulted in the routerLinkActive to not behave as expected, change was made to ensure query params with arrays are handled correctly

fixes #22223

PR Close #22666
@matsko matsko closed this in 828e5c1 Nov 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.