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

Update filter.js #11086

Closed
wants to merge 1 commit into from
Closed

Update filter.js #11086

wants to merge 1 commit into from

Conversation

ElephantRose
Copy link
Contributor

fix(filterFilter): solve issue #10991 with null property value when using objects with filter

Regarding the issue #10991 and the following discussion #10992

Regarding the issue angular#10991 and the following discussion angular#10992
Solve issue with null property value when using objects with filter.
@gkalpak
Copy link
Member

gkalpak commented Feb 17, 2015

Thx @ElephantRose !

Could you also add a test that would fail without the fix (and passes with the fix), so this is not accidentally broken again in the future ?

@petebacondarwin, should this fix go into 1.3 as well ?
(Note, it used to work (i.e. not throw an error) up to 1.3.5 (before the big filterFilter refactoring), but it throws an error since 1.3.6.)

@pkozlowski-opensource
Copy link
Member

This PR will need rebase as well, as it doesn't merge cleanly atm.

@gkalpak
Copy link
Member

gkalpak commented Feb 17, 2015

@ElephantRose, it would be also nice to follow the commit message guidelines, but it's not a blocker :)

@gkalpak
Copy link
Member

gkalpak commented Feb 17, 2015

@pkozlowski-opensource: Are you sure it needs rebasing ? How do you check this ?

@pkozlowski-opensource
Copy link
Member

@gkalpak it is GitHub that says this:

We can’t automatically merge this pull request.
Use the command line to resolve conflicts before continuing.

Text next to the gray merge icon

@gkalpak
Copy link
Member

gkalpak commented Feb 17, 2015

@pkozlowski-opensource, this was too obvious. I was hoping for some sophisticated git-fu moves 😞

Thx 😸

@ElephantRose
Copy link
Contributor Author

Hello,
@gkalpak
I wrote a unit test for the 1.3.12 buid here : JSFiddle
But is there any way to build an angular version with the fix without installing node, grunt, and anything else on my dev machine ?
Sadly, I don't really have much time to spent on this matter

@pkozlowski-opensource
It may look a stupid questions, but what do you mean by rebase ?
Sorry, I'm kinda a newbie to github ...

@gkalpak
Copy link
Member

gkalpak commented Feb 18, 2015

@ElephantRose, theoretically you don't need node/grunt etc. You can just put the extra test (it block) in 'test/ng/filter/filterSpec.js' (which is the spec file for filterFilter).

It is good to have the tools set up though, in order to be able to run the tests locally and verify that all is well.
But if you don't have time for that (or don't want to) it is not necessary (travis will run the tests anyway upon commit).

About rebasing, you can take a look at Submitting a PR in CONTRIBUTING.md.

@ElephantRose
Copy link
Contributor Author

@gkalpak ,
I 've added the test in the spec filter file and managed to successfully build it with travis.
(I had weird trailing space inserted that was messing up with build)

I did create a new pull request to replace this one.

I hope this would be ok this time :)

@gkalpak
Copy link
Member

gkalpak commented Feb 20, 2015

@ElephantRose: That's great ! I will take a look at the new PR shortly.
For the record, you didn't have to close this PR. You could just make the changes and force push to your branch (the PR gets updated automatically).

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants