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

Refactors PublisherToggle into redux component #9329

Merged
merged 1 commit into from
Jun 23, 2017
Merged

Refactors PublisherToggle into redux component #9329

merged 1 commit into from
Jun 23, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jun 7, 2017 •
edited by luixxiul
Loading

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 #9323

Auditors: @bridiver @bsclifton

Test Plan:

  • toggle should be hidden when payment is disabled
  • toggle should be hidden when you are on about page
  • toggle should be hidden when you are on non http/https site

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.18.x milestone Jun 7, 2017
@NejcZdovc NejcZdovc self-assigned this Jun 7, 2017
@NejcZdovc NejcZdovc mentioned this pull request Jun 7, 2017
Closed
51 tasks
bridiver
bridiver reviewed Jun 7, 2017
View reviewed changes
app/common/state/publisherState.js Outdated
* License, v. 2.0. If a copy of the MPL was not distributed with this file,
* You can obtain one at http://mozilla.org/MPL/2.0/. */

const Immutable = require('immutable')
Copy link
Collaborator

Choose a reason for hiding this comment

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

not sure if this file makes sense as a state helper. The state helpers should own a portion of the state and this is more of a set of utility functions that access other state helpers

Copy link
Contributor Author

@NejcZdovc NejcZdovc Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

it's not done yet 😃 but yeah, both functions used here get something from state (state as a first param, that's why I added them to state). Should we only put things into state folder if they manipulate state?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yes, they get something from the state, but they don't "own" part of the state. I think maybe better as util functions?

Copy link
Contributor Author

@NejcZdovc NejcZdovc Jun 7, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

sure I can move it into publisherUtil.js

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@NejcZdovc NejcZdovc force-pushed the redux/publisherToggle branch 4 times, most recently from 50528b2 to eedb2fa Compare June 8, 2017 01:08
@NejcZdovc NejcZdovc force-pushed the redux/publisherToggle branch 4 times, most recently from b3ce577 to 4a88708 Compare June 13, 2017 07:12
@NejcZdovc NejcZdovc modified the milestones: 0.19.x (Nightly Channel), 0.18.x (Developer Channel) Jun 13, 2017
@NejcZdovc
Copy link
Contributor Author

Let's merge this after #9415 is merged. This way unit test should stop failing.

@NejcZdovc NejcZdovc mentioned this pull request Jun 14, 2017
Closed
@NejcZdovc
Copy link
Contributor Author

PR rebased and ready for a review

@NejcZdovc
Copy link
Contributor Author

Unit test is failing, let me check out what is going on

@NejcZdovc
Copy link
Contributor Author

Unit test fixed

bridiver
bridiver reviewed Jun 23, 2017
View reviewed changes
app/renderer/components/navigation/urlBarIcon.js Outdated
@@ -115,6 +115,7 @@ class UrlBarIcon extends React.Component {
}

mergeProps (state, ownProps) {
console.log(state.get('tabs').toJS())
Copy link
Collaborator

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed

bridiver
bridiver approved these changes Jun 23, 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.

remove debug logging and then ++

@NejcZdovc
Refactors PublisherToggle into redux component
8717c97 
Resolves brave#9323

Auditors: @bridiver @bscfliton

Test Plan:
@NejcZdovc NejcZdovc merged commit b0145ac into brave:master Jun 23, 2017
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
refactoring/redux-component
Projects
None yet
Milestone
0.19.x (Beta Channel)
Development

Successfully merging this pull request may close these issues.
2 participants
@NejcZdovc @bridiver