Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Converts ClearBrowsingDataPanel into redux component #9457

Merged
merged 2 commits into from Jun 20, 2017
Merged

Converts ClearBrowsingDataPanel into redux component #9457

merged 2 commits into from Jun 20, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jun 14, 2017 •
edited

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Resolves #9440

Auditors: @bridiver @bsclifton

Test Plan:

  • check if you can clear history
  • check if toggles are preserved as you click clear
  • toggle one switch, click cancel and check that toggle was not preserved when you open it next time
  • check if ledger is still working correctly: ledger initi, ledger backup, ledger recovery

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@NejcZdovc NejcZdovc added this to the 0.19.x (Nightly Channel) milestone Jun 14, 2017
@NejcZdovc NejcZdovc self-assigned this Jun 14, 2017
@NejcZdovc NejcZdovc force-pushed the redux/clearBrowsingDataPanel branch 5 times, most recently from 9beae3a to c3d33de Compare June 15, 2017 08:01
NejcZdovc
NejcZdovc commented Jun 15, 2017
View reviewed changes
app/ledger.js Outdated
@@ -168,7 +168,8 @@ const doAction = (action) => {
break

case appConstants.APP_ON_CLEAR_BROWSING_DATA:
if (action.clearDataDetail.get('browserHistory') && !getSetting(settings.PAYMENTS_ENABLED)) reset(true)
// TODO
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bsclifton @bridiver I would need your help here. What we need to add here is state (now we only get action) so that you can check it the same way we do it for sites. How should we address this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking to add state directly in same as we do with other reducers. One thing that we can also do is get state only in this case with getState

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Jun 15, 2017 •
edited

Except this ledger problem everything else is done

This was referenced Jun 15, 2017
Closed
Closed
@NejcZdovc NejcZdovc force-pushed the redux/clearBrowsingDataPanel branch 3 times, most recently from 51127ac to 4464958 Compare June 16, 2017 15:28
@NejcZdovc
Copy link
Contributor Author

Don't squash commits, because second one is quite special and I would keep it separated.

@NejcZdovc NejcZdovc force-pushed the redux/clearBrowsingDataPanel branch 2 times, most recently from df8b6b3 to aeeca3d Compare June 19, 2017 22:00
bridiver
bridiver reviewed Jun 20, 2017
View reviewed changes
app/browser/menu.js
if (action.clearDataDetail.get('browserHistory')) {
appDispatcher.waitFor([appStore.dispatchToken], () => {
appDispatcher.waitFor([appStore.dispatchToken], () => {
if (appStore.getState().getIn(['clearBrowsingDataDefaults', 'browserHistory'])) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

why the change here from action.clearDataDetail to checking the state?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because we are not sending anything in the action anymore. We have everything that we need directly in the store already. We are saving data here already https://github.com/brave/browser-laptop/pull/9457/files#diff-23ca389e2bcb77191b5a9c10900eb3a3R697

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actions has no params https://github.com/brave/browser-laptop/pull/9457/files#diff-5fea3af223318beea03d50c8ee3de64dR591

bridiver
bridiver approved these changes Jun 20, 2017
View reviewed changes
Copy link
Collaborator

@bridiver bridiver left a comment

Choose a reason for hiding this comment

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

++

@NejcZdovc NejcZdovc force-pushed the redux/clearBrowsingDataPanel branch from aeeca3d to 17057bd Compare June 20, 2017 16:04
@NejcZdovc NejcZdovc merged commit 5cb22be into brave:master Jun 20, 2017
@cezaraugusto cezaraugusto mentioned this pull request Aug 12, 2017
Merged
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bridiver bridiver bridiver approved these changes

@bsclifton bsclifton Awaiting requested review from bsclifton

Assignees

@NejcZdovc NejcZdovc

Labels
PR/needs-QA-attention ☕ refactoring/redux-component
Projects
None yet
Milestone
0.19.x (Beta Channel)
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@NejcZdovc @bridiver