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

Add BrowserButton component #8560

Merged
merged 1 commit into from
May 5, 2017
Merged

Add BrowserButton component #8560

merged 1 commit into from
May 5, 2017

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Apr 28, 2017

  • 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, @NejcZdovc
Close: #8568

Note: This PR removed "whiteButton" for new buttons since at the point we want to allow dark-UI/theming, it will not necessarily be white.

Test Plan:

  • No breaking tests should be found regarding buttons
  • Buttons visible in below images should do their expected actions
  • Buttons visible in below images should not have UI regressions
  • about:styles should have primaryColor, secondaryColor (previous whiteButton) and wideButton aesthetically ok.

Buttons refactored

  • All buttons inside Sync tab

screen shot 2017-04-28 at 9 29 08 pm

screen shot 2017-04-28 at 9 28 18 pm

screen shot 2017-04-28 at 9 19 00 pm

screen shot 2017-04-28 at 9 18 54 pm

screen shot 2017-04-28 at 9 17 29 pm

screen shot 2017-04-28 at 9 16 31 pm

screen shot 2017-04-28 at 8 12 52 pm

screen shot 2017-04-28 at 8 11 37 pm

screen shot 2017-04-28 at 8 07 48 pm

screen shot 2017-04-28 at 7 53 09 pm

Buttons changed as side effect

We had a global selector that changed every button that had Button className. I replaced that with normalizeButton so standard HTML5 buttons weren't affected. For reference:

screen shot 2017-04-28 at 9 48 14 pm

screen shot 2017-04-29 at 12 17 23 am

screen shot 2017-04-29 at 12 17 07 am

screen shot 2017-04-29 at 12 16 44 am

}
},
// applies for primary and white buttons
browserButton_default: {
Copy link
Contributor

Choose a reason for hiding this comment

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

nice to see the modifed BEM style :-)

@luixxiul
Copy link
Contributor

Nice work. As a reminder, would you please apply the change to about:styles? Thanks!

@cezaraugusto
Copy link
Contributor Author

sure, thanks for checking ;)

@cezaraugusto
Copy link
Contributor Author

cezaraugusto commented Apr 29, 2017

btw @luixxiul I added #8568 as an issue to track this PR since it doesn't refactor all components and removed #7168 reference (added you there as assignee as well) so we can keep tracking until every button uses BrowserButton. Then <Button> component can be removed.

There are too many files using (now legacy) Button that the likeliness of regression was too high, so I decided to keep it gradual.

@cezaraugusto cezaraugusto added this to the 0.15.2 milestone Apr 29, 2017
@cezaraugusto cezaraugusto self-assigned this Apr 29, 2017
@luixxiul
Copy link
Contributor

FYI: reset Sync will be replaced with the primaryButton after #8044 is merged.

@luixxiul
Copy link
Contributor

luixxiul commented May 2, 2017

@cezaraugusto I left some comments.

@cezaraugusto
Copy link
Contributor Author

thanks, updated

@bbondy
Copy link
Member

bbondy commented May 5, 2017

Since comments were addressed, merging.

@bbondy bbondy merged commit ac63fb4 into brave:master May 5, 2017
@luixxiul
Copy link
Contributor

luixxiul commented May 6, 2017

As I removed .paymentsContainer the margin between the buttons on "Back up your Brave wallet" dialog have regressed. The specification here seems to be no longer applied: https://github.com/brave/browser-laptop/blob/master/less/button.less#L180

clipboard01

Since on the other dialogs the margin exists, I believe something is inconsistent between them. I'll tackle that.

@srirambv
Copy link
Collaborator

srirambv commented May 8, 2017

@luixxiul is that expected behaviour for the backup wallet modal?

@luixxiul
Copy link
Contributor

luixxiul commented May 8, 2017

No, and it was fixed already

@cezaraugusto cezaraugusto deleted the components/browserbutton branch July 25, 2017 07:24
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.

4 participants