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

Android browser develop #97

Merged
merged 6 commits into from Aug 6, 2018

Conversation

@mai-cliqz
Copy link
Contributor

@mai-cliqz mai-cliqz commented Jun 11, 2018

  • Have you followed the guidelines in CONTRIBUTING.md?
  • Have you checked to ensure there aren't other open Pull Requests for the same update/change?
  • Have you added an explanation of what your changes do?
  • Does your submission pass tests?
  • Did you lint your code prior to submission?
@mai-cliqz mai-cliqz requested review from trickpattyFH20, zarembsky and ghostery/ghostery as code owners Jun 11, 2018
@luciancor
Copy link
Contributor

@luciancor luciancor commented Jun 11, 2018

@christophertino can you guys please take a look at this PR? This is the Android Panel which should appear for the Firefox for Android users

@mai-cliqz mai-cliqz force-pushed the mai-cliqz:android_browser_develop branch from 98de65c to 495c065 Jun 12, 2018
@mai-cliqz mai-cliqz force-pushed the mai-cliqz:android_browser_develop branch from dba44ed to 26ba8cc Jun 19, 2018
@mai-cliqz mai-cliqz force-pushed the mai-cliqz:android_browser_develop branch 2 times, most recently from 6fed6df to e4e7dd3 Jul 18, 2018
@christophertino
Copy link
Member

@christophertino christophertino commented Jul 19, 2018

@mai-cliqz this looks solid. I think we could import some of the utils like msg.js and utils.js from the panel folder like we do for setup, rather than having duplicated code. See here

We also need to add the license header to all the new files.

++ @IAmThePan for review

@christophertino christophertino requested a review from IAmThePan Jul 19, 2018
@mai-cliqz mai-cliqz force-pushed the mai-cliqz:android_browser_develop branch from 4c8e798 to cefcb09 Jul 20, 2018
@mai-cliqz mai-cliqz force-pushed the mai-cliqz:android_browser_develop branch from cefcb09 to 74dbcae Jul 20, 2018
@mai-cliqz
Copy link
Contributor Author

@mai-cliqz mai-cliqz commented Jul 20, 2018

Thank you very much for reviewing @christophertino. I have applied some changes based on your feedback. Please have a look again.

@IAmThePan
Copy link
Contributor

@IAmThePan IAmThePan commented Jul 20, 2018

@mai-cliqz There is a lot here. Two things that jump out at me are:

  1. We recently made some changes to the blocking logic in app/panel/utils/blocking.js (d5d26e1). Make sure those changes are implemented in the Android panel.
  2. I think you need to add a line to webpack.config.js ${RM} ./dist/panel_android_ui.js (around line 48)
@mai-cliqz
Copy link
Contributor Author

@mai-cliqz mai-cliqz commented Jul 26, 2018

@IAmThePan Thank you very much for your time.

  1. We discussed together with our UX and Vincent about the changes we made in blocking.js. So we decided not to change anything for now. Anyway the logic on desktop and mobile extension are a bit different.
  2. We don't add the line to webpack.config.js because we don't want to remove the file after building. Since the file is needed for the UI to function.
@christophertino christophertino merged commit ce5420e into ghostery:develop Aug 6, 2018
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
jsignanini added a commit that referenced this pull request Nov 20, 2018
* Update package.json

* Allow getting cliqzModuleData from specified tab

* Control Center for Android

* Allow user to click on a category's checkbox to block/unblock all trackers inside

* Add the license header to all the new files
jsignanini added a commit that referenced this pull request Nov 20, 2018
* Update package.json

* Allow getting cliqzModuleData from specified tab

* Control Center for Android

* Allow user to click on a category's checkbox to block/unblock all trackers inside

* Add the license header to all the new files
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants