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

Cleanup of LoginRequired #9493

Merged
merged 1 commit into from
Jun 23, 2017
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Jun 15, 2017

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

Auditors: @bsclifton

Test Plan:

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 15, 2017
@NejcZdovc NejcZdovc self-assigned this Jun 15, 2017
@NejcZdovc NejcZdovc requested a review from bsclifton June 15, 2017 12:43
@NejcZdovc NejcZdovc force-pushed the redux/loginRequired branch from 01c2bf5 to b57a646 Compare June 23, 2017 13:23
Resolves brave#9446

Auditors: @bsclifton

Test Plan:
@NejcZdovc NejcZdovc force-pushed the redux/loginRequired branch from b57a646 to 7cf3505 Compare June 23, 2017 13:27
</CommonFormButtonWrapper>
</CommonForm>
</Dialog>
}
}

LoginRequired.propTypes = { frameProps: PropTypes.object }
module.exports = LoginRequired
Copy link
Member

Choose a reason for hiding this comment

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

nit: I prefer this at the end of a file

Copy link
Contributor Author

@NejcZdovc NejcZdovc Jun 23, 2017

Choose a reason for hiding this comment

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

In all components we have this like that, so I would leave it for now and we can move it in all components to the bottom in a different PR

Copy link
Member

Choose a reason for hiding this comment

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

ok I'm not concerned either way, was just noting.

Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed it as well and was trying to re-move the change.

Copy link
Contributor

Choose a reason for hiding this comment

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

@NejcZdovc please document the rule on the wiki page for refactoring work, thanks. CC @cezaraugusto

@bbondy bbondy merged commit 4f4a0b9 into brave:master Jun 23, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants