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

Apply BrowserButton to clipboardButton #11374

Merged
merged 1 commit into from
Nov 21, 2017
Merged

Apply BrowserButton to clipboardButton #11374

merged 1 commit into from
Nov 21, 2017

Conversation

luixxiul
Copy link
Contributor

@luixxiul luixxiul commented Oct 8, 2017

  • by applying the modified BEM style to dedupe properties
  • by replacing span with browserButton
  • by setting clipboard fa icon by default

Closes #11373

Auditors:

Test Plan:

  1. Open about:brave
  2. Click the clipboard button
  3. Make sure the data is copied

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.

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

@luixxiul luixxiul added misc/button polish Nice to have — usually related to front-end/visual tasks. refactoring/aphrodite labels Oct 8, 2017
@luixxiul luixxiul added this to the 0.21.x (Nightly Channel) milestone Oct 8, 2017
@luixxiul luixxiul self-assigned this Oct 8, 2017
@luixxiul luixxiul mentioned this pull request Oct 9, 2017
@codecov-io
Copy link

codecov-io commented Oct 9, 2017

Codecov Report

Merging #11374 into master will decrease coverage by 0.65%.
The diff coverage is 37.5%.

@@            Coverage Diff             @@
##           master   #11374      +/-   ##
==========================================
- Coverage   52.67%   52.01%   -0.66%     
==========================================
  Files         269      269              
  Lines       25602    25537      -65     
  Branches     4085     4070      -15     
==========================================
- Hits        13485    13284     -201     
- Misses      12117    12253     +136
Flag Coverage Δ
#unittest 52.01% <37.5%> (-0.66%) ⬇️
Impacted Files Coverage Δ
...ment/addFundsDialog/steps/addFundsWizardAddress.js 31.25% <ø> (-1.06%) ⬇️
app/renderer/components/common/clipboardButton.js 40% <37.5%> (+4.51%) ⬆️
app/common/state/ledgerState.js 45.56% <0%> (-12.1%) ⬇️
app/browser/api/ledger.js 30.83% <0%> (-8.33%) ⬇️
js/actions/appActions.js 16.51% <0%> (-1.81%) ⬇️
js/constants/windowConstants.js 100% <0%> (ø) ⬆️
app/browser/tabs.js 25.82% <0%> (ø) ⬆️
js/actions/windowActions.js 4.9% <0%> (+0.04%) ⬆️
js/stores/windowStore.js 27.58% <0%> (+0.56%) ⬆️
... and 1 more

@cezaraugusto
Copy link
Contributor

cezaraugusto commented Oct 13, 2017

blocking for now as this affects add funds on 0.19.x

@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel), Backlog Oct 25, 2017
- Apply the modified BEM style to dedupe properties
- Add copyToClipboard.title to preferences.properties
- Replace span with browserButton
- Set copyToClipboard.title by default (because it is what we usually want to set)

Closes #11373

Auditors:

Test Plan:
1. Open about:brave
2. Click the clipboard button
3. Make sure the data is copied
4. Open about:preferences#payments
5. Click add funds
6. Click any currency icon
7. Make sure 'Copy to clipboard' is displayed on hover
@luixxiul luixxiul changed the title Polish clipboardButton Apply BrowserButton to clipboardButton Nov 17, 2017
@luixxiul luixxiul removed the polish Nice to have — usually related to front-end/visual tasks. label Nov 17, 2017
@cezaraugusto cezaraugusto added 0.22.x issue first seen in 0.22.x and removed PR/blocked labels Nov 21, 2017
@cezaraugusto
Copy link
Contributor

one thing is that with this move we lost the pointer as the cursor. Could make a workaround for this? otherwise this lgtm

@luixxiul
Copy link
Contributor Author

That will be taken care of https://github.com/brave/browser-laptop/pull/10951/files#diff-0ff973318abdc5c07f55cefe86bd5ed7R110 of #10951. Is it OK to wait until that?

Copy link
Contributor

@cezaraugusto cezaraugusto left a comment

Choose a reason for hiding this comment

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

++

@cezaraugusto cezaraugusto merged commit bbab253 into brave:master Nov 21, 2017
@luixxiul luixxiul deleted the polish-clipboardButton branch November 21, 2017 15:03
@luixxiul luixxiul modified the milestones: Triage Backlog, 0.22.x (Nightly Channel) Nov 21, 2017
@luixxiul luixxiul removed the request for review from bsclifton November 21, 2017 15:15
@bbondy bbondy modified the milestones: 0.22.x (Developer Channel), 0.23.x (Nightly Channel) Feb 25, 2018
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.

5 participants