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.

Sign up for GitHub

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

Jump to bottom

Project C.A.T.S. #82

Merged
merged 12 commits into from
Sep 15, 2016
Merged

Project C.A.T.S. #82

merged 12 commits into from
Sep 15, 2016

Conversation

tomasko126
Copy link
Member

  • Added detection code, whether CB has been installed from CWS/homepage or the dedicated page for C.A.T.S. project
  • When installed from a special link, “Project C.A.T.S.” mode will be enabled (this means that CB will be subscribed to another Flickr channel with cats from the project)

@tomasko126
Initial work on Project CATS feature
7be9a54 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Merge branch 'master' into project-cats
d2c1e4d 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Resolve merge conflict
764d888 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Improve the detection of a CATS user, minor enhancements
fff2d26 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Remove localstorage key after running a content-script
d3352c6 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Add channels to the Options page by default, but leave them disabled …
0d2bb57 
…when Project CATS is enabled

Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Merge branch 'master' into project-cats
e48b36f 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126 tomasko126 added this to the 1.4.1-1.4.x milestone Sep 15, 2016
@tomasko126 tomasko126 added enhancement This is an enhancement to the extension pr-high This is a high-priority issue. If you want to help out, try one of these if you can! labels Sep 15, 2016
itskdog
itskdog requested changes Sep 15, 2016
View reviewed changes
Copy link
Member

@itskdog itskdog left a comment

Choose a reason for hiding this comment

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

There's a few things that need changing. Let me know when it's ready to review again.

catblock/contentscript.js
} catch(e) {
return undefined;
}
return intFor(el.getAttribute(prop) || window.getComputedStyle(el)[prop]);
Copy link
Member

Choose a reason for hiding this comment

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

Is this not part of another PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kpeckett This was changed as part of a merge commit from master to project-cats branch.

Copy link
Member

Choose a reason for hiding this comment

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

Try rebasing next time: https://www.atlassian.com/git/tutorials/merging-vs-rebasing/ avoids extra commits appearing.

catblock/options/catblock.js
@@ -7,6 +7,9 @@ function mayDelete(channelData) {
if (channelData.name === "TheCatsOfCatBlockUsersChannel") {
return false;
}
if (channelData.name === "TheCatsOfProjectCATS") {
return false;
}
Copy link
Member

Choose a reason for hiding this comment

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

Could this be changed to a switch statement?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kpeckett I will update the code accordingly.

js/cats.js
if (self.isEnabled() === undefined && sessionstorage_get("installed")) {
sessionstorage_set("installed"); // present with a new user

var installURL = "https://catblock.tk/installed.html";
Copy link
Member

Choose a reason for hiding this comment

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

Why not a local page?

Copy link
Member Author

Choose a reason for hiding this comment

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

@kpeckett When a user installs CatBlock from the special link, some data to localStorage will be saved (project-cats will be saved as true).
After we open a post-installation URL, CatBlock will try to get project-cats data from localStorage.

When CB detects there is not such an entry, it won't enable "Project CATS" mode. When there is, it will send a message to CatBlock to enable "Project CATS" mode.

js/background.js
// Init "Project CATS"
CATS.init(function(enabled) {
channels = new Channels();
console.log("initialized ", enabled);
Copy link
Member

Choose a reason for hiding this comment

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

Leftover debugging code.

Copy link
Member Author

Choose a reason for hiding this comment

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

@kpeckett I will delete debugging code accordingly.

@itskdog itskdog assigned tomasko126 and unassigned itskdog Sep 15, 2016
@tomasko126
Removed logs and other changes requested by Kieran
fd6edbe 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Merge branch 'master' into project-cats
e09a155 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Fix Codacy issues
6cf8951 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Copy link
Member Author

@kpeckett Is this ready to be merged?

itskdog
itskdog reviewed Sep 15, 2016
View reviewed changes
catblock/options/catblock.js
switch (channelData.name) {
case "AprilFoolsCatsChannel": return false;
case "TheCatsOfCatBlockUsersChannel": return false;
case "TheCatsOfProjectCATS": return false;
Copy link
Member

@itskdog itskdog Sep 15, 2016 •
edited
Loading

Choose a reason for hiding this comment

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

I was thinking more along the lines of:

switch (channelData.name) {
        case "AprilFoolsCatsChannel":
        case "TheCatsOfCatBlockUsersChannel":
        case "TheCatsOfProjectCATS":
        return false;
        break;
        defualt:
        return true;
}

itskdog
itskdog requested changes Sep 15, 2016
View reviewed changes
Copy link
Member

@itskdog itskdog left a comment

Choose a reason for hiding this comment

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

You sort of got the idea with the case statements, but it can be made to look cleaner.

@tomasko126
Adjust switch block
20bb3e4 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Remove break;
c203339 
Signed-off-by: Tomáš Taro <tomas@getadblock.com>
@tomasko126
Copy link
Member Author

tomasko126 commented Sep 15, 2016 •
edited
Loading

@kpeckett Codacy is wrong here with the intendation. Anyway, I've made changes you've requested. Requesting PR merge approval.

@itskdog itskdog merged commit 44ca859 into master Sep 15, 2016
@tomasko126 tomasko126 deleted the project-cats branch September 16, 2016 00:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@itskdog itskdog itskdog approved these changes

Assignees

@tomasko126 tomasko126

Labels
enhancement This is an enhancement to the extension pr-high This is a high-priority issue. If you want to help out, try one of these if you can!
Projects
None yet
Milestone
1.4.1-1.4.x
Development

Successfully merging this pull request may close these issues.
2 participants
@tomasko126 @itskdog