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

Moves buttons from navigator and navigationBar into separate components #10355

Merged
merged 1 commit into from
Aug 13, 2017
Merged

Moves buttons from navigator and navigationBar into separate components #10355

merged 1 commit into from
Aug 13, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Aug 9, 2017 •
edited by cezaraugusto
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 #10354
Addresses #9283

Auditors: @cezaraugusto @bsclifton

Test Plan:

  • check if reload button is working
  • check if stop button is working
  • check if forward/back buttons are working
  • check if long press forward/back buttons are working
  • check if bookmark button is working (add/remove)

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
Moves buttons from navigator and navigationBar into seperate components
3a830f4 
Resolves brave#10354

Auditors: @cezaraugusto @bsclifton

Test Plan:
- check if reload button is working
- check if stop button is working
- check if forward/back buttons are working
- check if long press forward/back buttons are working
- check if bookmark button is working (add/remove)
@NejcZdovc NejcZdovc added this to the 0.21.x (Nightly Channel) milestone Aug 9, 2017
@NejcZdovc NejcZdovc self-assigned this Aug 9, 2017
@luixxiul
Copy link
Contributor

luixxiul commented Aug 9, 2017

please refer to the original commit in the commit message if you don't mind. thanks.

luixxiul
luixxiul reviewed Aug 9, 2017
View reviewed changes
app/renderer/components/navigation/buttons/bookmarkButton.js
bookmarkButton: true,
removeBookmarkButton: !!this.props.bookmarkKey,
withHomeButton: getSetting(settings.SHOW_HOME_BUTTON),
normalizeButton: true
Copy link
Contributor

Choose a reason for hiding this comment

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

will this be replaced with <NormalizedButton> later?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this PR just moves things around and don't change any functionalities or introduce new ones

Copy link
Contributor

Choose a reason for hiding this comment

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

ok understood.

@NejcZdovc
Copy link
Contributor Author

NejcZdovc commented Aug 9, 2017 •
edited
Loading

@luixxiul which one is the original commit? this is just a pre-work for your PR

@codecov-io
Copy link

codecov-io commented Aug 9, 2017 •
edited
Loading

Codecov Report

Merging #10355 into master will increase coverage by 0.24%.
The diff coverage is 78.21%.

@@            Coverage Diff             @@
##           master   #10355      +/-   ##
==========================================
+ Coverage   53.67%   53.91%   +0.24%     
==========================================
  Files         238      244       +6     
  Lines       21049    21189     +140     
  Branches     3256     3268      +12     
==========================================
+ Hits        11297    11423     +126     
- Misses       9752     9766      +14
Flag Coverage Δ
#unittest 53.91% <78.21%> (+0.24%) ⬆️
Impacted Files Coverage Δ
.../components/navigation/buttons/navigationButton.js 100% <100%> (ø)
...nderer/components/navigation/buttons/homeButton.js 36.84% <100%> (ø)
...pp/renderer/components/navigation/navigationBar.js 98.71% <100%> (+16.45%) ⬆️
...ponents/navigation/buttons/windowCaptionButtons.js 33.33% <100%> (ø)
...nderer/components/navigation/buttons/stopButton.js 45.45% <42.85%> (ø)
app/renderer/components/navigation/navigator.js 73.68% <75%> (+3.25%) ⬆️
...erer/components/navigation/buttons/reloadButton.js 77.41% <76.66%> (ø)
...nderer/components/navigation/buttons/backButton.js 78.57% <78.57%> (ø)
...rer/components/navigation/buttons/forwardButton.js 78.57% <78.57%> (ø)
...er/components/navigation/buttons/bookmarkButton.js 82.35% <81.81%> (ø)
... and 8 more

@luixxiul
Copy link
Contributor

luixxiul commented Aug 9, 2017

@luixxiul which one is original commit? this is just a pre-work for your PR

ok nvm.

cezaraugusto
cezaraugusto approved these changes Aug 13, 2017
View reviewed changes
Copy link
Contributor

@cezaraugusto cezaraugusto left a comment •
edited
Loading

Choose a reason for hiding this comment

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

test plan works, no regressions in automated tests, ++

@cezaraugusto cezaraugusto merged commit 761502d into brave:master Aug 13, 2017
@NejcZdovc NejcZdovc mentioned this pull request Aug 13, 2017
Closed
bsclifton
bsclifton reviewed Aug 13, 2017
View reviewed changes
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

++

@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@bsclifton bsclifton bsclifton left review comments

@luixxiul luixxiul luixxiul left review comments

@cezaraugusto cezaraugusto cezaraugusto approved these changes

Assignees

@NejcZdovc NejcZdovc

Labels
misc/button refactoring
Projects
None yet
Milestone
0.20.x (Release Channel)
Development

Successfully merging this pull request may close these issues.
6 participants
@NejcZdovc @luixxiul @codecov-io @cezaraugusto @bsclifton @bbondy