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

Incorporated filter objects to a single JSON object #47

Merged
merged 1 commit into from Jan 1, 2016

Conversation

Projects
None yet
3 participants
@CodeMaxx
Copy link
Contributor

CodeMaxx commented Jan 1, 2016

fix for #41
== issue not fixed yet

Review on Reviewable

@hejbot

This comment has been minimized.

Copy link

hejbot commented Jan 1, 2016

Thanks for the pull request, and welcome!

The following reviewer was randomly selected for this PR: @jgraham
And the following additional people are watching it:

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Review status: 0 of 3 files reviewed at latest revision, 6 unresolved discussions.


angular_scripts.js, line 107 [r2] (raw file):
you need to change the parameters here too


angular_scripts.js, line 109 [r2] (raw file):
and here as well


angular_scripts.js, line 143 [r2] (raw file):
Good start, maybe we could move this to $scope and add the contents of the other $scope objects to this JSON. Also, you will need to make changes to index.html to allow this to work.


logcruncher.js, line 16 [r3] (raw file):
nit: Add a space on either side of = and <


logcruncher.js, line 17 [r3] (raw file):
nit: Add a space on either side of ===


logcruncher.js, line 32 [r3] (raw file):
nit: Add the space here as well. Sorry this isn't mentioned in the issue, but do it here.


Comments from the review on Reviewable.io

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 1, 2016

Whats the problem in line 143?

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Review status: 0 of 3 files reviewed at latest revision, 9 unresolved discussions.


angular_scripts.js, line 127 [r2] (raw file):
So we want to move this object completely inside $scope.filter


angular_scripts.js, line 128 [r2] (raw file):
So we want to move this object completely inside $scope.filter


angular_scripts.js, line 140 [r2] (raw file):
So we want to move this object completely inside $scope.filter


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Review status: 0 of 4 files reviewed at latest revision, 5 unresolved discussions.


angular_scripts.js, line 143 [r2] (raw file):
so put this on $scope and remove the other 3 variables


index.html, line 95 [r4] (raw file):
Change this ng-model as well


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Review status: 0 of 4 files reviewed at latest revision, 7 unresolved discussions.


angular_scripts.js, line 139 [r5] (raw file):
nit: Add a space after :


angular_scripts.js, line 141 [r5] (raw file):
nit: Add a space here too. Lots of indentation and trailing space errors here. Please see other JSON and then update


angular_scripts.js, line 221 [r5] (raw file):
Make this $scope.filter


angular_scripts.js, line 245 [r5] (raw file):
Make this $scope.filter


angular_scripts.js, line 253 [r5] (raw file):
Make this $scope.filter


angular_scripts.js, line 257 [r5] (raw file):
Make this $scope.filter


angular_scripts.js, line 264 [r5] (raw file):
Make this $scope.filter


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.


angular_scripts.js, line 142 [r6] (raw file):
nit: So we use 2 space and not tab as indentation. Please fix this. Also, this line and the next have trailing spaces. Also fix the indent for the next line


angular_scripts.js, line 222 [r6] (raw file):
nit: Move these parameters to the previous line. Make sure you have a space after ,


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.


angular_scripts.js, line 139 [r5] (raw file):
We are using two spaces instead of tabs. This line has 3 spaces, please make it 4. The same for four lines below. I'm sorry we're so fussy about these indentation fixes. In the end, it has to be consistent and readable. Please bear with me. You won't make these mistakes in the future if you take this to completion.


angular_scripts.js, line 144 [r7] (raw file):
trailing space here.


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Review status: 0 of 4 files reviewed at latest revision, 3 unresolved discussions.


LovefieldService.js, line 289 [r7] (raw file):
nit: Ah small thing here too, sorry. Put this in the previous line, space after ,


Comments from the review on Reviewable.io

@CodeMaxx

This comment has been minimized.

Copy link
Contributor

CodeMaxx commented Jan 1, 2016

@martiansideofthemoon Thanxx for the review. :D I'll take care of the indentation next time on.

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Reviewed 1 of 3 files at r4, 1 of 2 files at r7, 2 of 2 files at r8.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from the review on Reviewable.io

@martiansideofthemoon

This comment has been minimized.

Copy link
Contributor

martiansideofthemoon commented Jan 1, 2016

Looks good to me. r+.
@jgraham , Anything else is needed?
@CodeMaxx thanks for your contribution. 😄 Please rebase (we've merged the license PR) and squash. Hoping to see your contributions in the future.

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:Cleanup branch from d666900 to fab82c7 Jan 1, 2016

@CodeMaxx CodeMaxx force-pushed the CodeMaxx:Cleanup branch from fab82c7 to 57a0c81 Jan 1, 2016

martiansideofthemoon pushed a commit that referenced this pull request Jan 1, 2016

Kalpesh Krishna
Merge pull request #47 from CodeMaxx/Cleanup
Incorporated filter objects to a single JSON object, change == to ===

@martiansideofthemoon martiansideofthemoon merged commit 1f41d66 into mozilla:master Jan 1, 2016

1 check passed

code-review/reviewable Review complete: all files reviewed, all discussions resolved
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment