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

move actionButton to Aphrodite #9164

Merged
merged 1 commit into from
Jun 6, 2017
Merged

move actionButton to Aphrodite #9164

merged 1 commit into from
Jun 6, 2017

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented May 31, 2017 •
edited
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).

Auditors: @luixxiul

Close #9163

Test Plan:

  1. go to about:certerror
  2. buttons should have no visual regressions
  3. go to aihsdiuahsdis.cisdhjfsidf or any other invalid url (which will lead you to about:error page)
  4. buttons should have no visual regressions
  5. go to about:styles
  6. action button should have no visual regressions
  7. go to brave.com, block scripts
  8. click no script button
  9. buttons should have no visual regressions

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

@cezaraugusto cezaraugusto added this to the 0.18.x milestone May 31, 2017
@cezaraugusto cezaraugusto self-assigned this May 31, 2017
@cezaraugusto cezaraugusto requested a review from luixxiul May 31, 2017 04:16
luixxiul
luixxiul reviewed Jun 2, 2017
View reviewed changes
app/renderer/components/common/browserButton.js Outdated
this.props.notificationItem && styles.browserButton_notificationItem,
// actionItem is just subtleItem with a blue background
this.props.actionItem &&
[styles.browserButton_default, styles.browserButton_subtleItem, styles.browserButton_actionItem]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move the 3 lines above to a line between L16-L17, subtleItem and extensionItem.

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 agree

@luixxiul
Copy link
Contributor

luixxiul commented Jun 2, 2017

go to about:error

Change looks good, while opening about:error directly shows a blank tab without buttons.

luixxiul
luixxiul reviewed Jun 2, 2017
View reviewed changes
Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

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

Feedback left.

@cezaraugusto
Copy link
Contributor Author

bad str, you should be lead to the err page typing an invalid url like aihsdiuahsdis.cisdhjfsidf, I just updated STR.

luixxiul
luixxiul approved these changes Jun 3, 2017
View reviewed changes
Copy link
Contributor

@luixxiul luixxiul left a comment

Choose a reason for hiding this comment

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

LGTM.

@cezaraugusto cezaraugusto merged commit 2fc1da6 into brave:master Jun 6, 2017
@cezaraugusto cezaraugusto mentioned this pull request Jun 6, 2017
Closed
@cezaraugusto cezaraugusto deleted the aphrodite/9163 branch July 25, 2017 07:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@luixxiul luixxiul luixxiul approved these changes

Assignees

@cezaraugusto cezaraugusto

Labels
misc/button refactoring/aphrodite release-notes/exclude
Projects
None yet
Milestone
0.18.x Release
Development

Successfully merging this pull request may close these issues.
2 participants
@cezaraugusto @luixxiul