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

Adds confirmation dialog upon clicking the trash icon in the ledger table. #13128

Closed
wants to merge 25 commits into from
Closed

Adds confirmation dialog upon clicking the trash icon in the ledger table. #13128

wants to merge 25 commits into from

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Feb 13, 2018

Fixes Issue #11164

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.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

  1. Visit some sites
  2. Open about:payments
  3. Click a trash icon
  4. Ensure that the appearing confirmation dialogue, depending on the choice, allows the removal of the site or does nothing.

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

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

Two translations added under the name: banSiteConfirmation. Mechanism for creating the confirmation dialogue mimics that shown in the syncTable component. Ref: here

@ryanml
Copy link
Contributor Author

ryanml commented Feb 13, 2018

@bsclifton I hope I got the PR title right this time ;)

I hope that the dialogue provided in this PR is appropriate, should a change in language work better, let me know and I can update those locales.

cc: @NejcZdovc @luixxiul

@bsclifton
Copy link
Member

@bradleyrichter how does the wording for this look?
Are you sure you want to delete this site?

screen shot 2018-02-16 at 11 24 11 am

@codecov-io
Copy link

codecov-io commented Feb 16, 2018

Codecov Report

Merging #13128 into master will decrease coverage by 0.03%.
The diff coverage is 33.94%.

@@            Coverage Diff             @@
##           master   #13128      +/-   ##
==========================================
- Coverage   56.18%   56.14%   -0.04%     
==========================================
  Files         279      279              
  Lines       27873    27876       +3     
  Branches     4560     4561       +1     
==========================================
- Hits        15660    15651       -9     
- Misses      12213    12225      +12
Flag Coverage Δ
#unittest 56.14% <33.94%> (-0.04%) ⬇️
Impacted Files Coverage Δ
...renderer/components/preferences/payment/history.js 34.17% <ø> (ø) ⬆️
app/locale.js 35.77% <ø> (ø) ⬆️
js/constants/appConfig.js 100% <ø> (ø) ⬆️
js/actions/appActions.js 19.67% <ø> (ø) ⬆️
app/renderer/components/styles/global.js 100% <ø> (ø) ⬆️
app/browser/api/ledgerNotifications.js 75.33% <0%> (ø) ⬆️
app/browser/reducers/windowsReducer.js 80.85% <100%> (ø) ⬆️
app/cmdLine.js 39.42% <100%> (ø) ⬆️
js/data/siteHacks.js 45.58% <100%> (ø) ⬆️
app/common/state/pinnedSitesState.js 77.27% <100%> (ø) ⬆️
... and 26 more

@bsclifton bsclifton removed their request for review February 16, 2018 22:28
@bsclifton
Copy link
Member

bsclifton commented Feb 16, 2018

Will review of this up to @NejcZdovc 😄 There are some planned changes for this UI (we may want to make those first): #13056

@@ -24,6 +24,7 @@ autoplay=Autoplay Media
autoSuggestSites=auto-incluir
backupLedger=Copia de seguridad de tu cartera
balanceRecovered=Se ha recuperado un total de {{balance}} y se han transferido a tu cartera Brave.
banSiteConfirmation=¿Seguro que quieres eliminar este sitio?
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this line. We are doing translations via Transifex https://github.com/brave/browser-laptop/blob/master/docs/translations.md

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@NejcZdovc apologies there, i've removed this line.

@NejcZdovc NejcZdovc self-requested a review February 19, 2018 05:42
@NejcZdovc
Copy link
Contributor

@ryanml can you please squash commits? Then we are good to go

MargarytaChepiga and others added 10 commits February 18, 2018 22:50
…'wasted' time, total time and number of invocations

This option counts:
- all time that is spent in all component mergeProps functions that results in no properties actually changing (and therefore no render), every 1 second. This is an important metric to improve since this is blocking the renderer window UI thread, and the time is likely spent re-computing functions even though the state input to those functions has not changed.
- total time for each Component-type spent in mergeProps, output every 10 seconds
- number of invocations for each Component-type mergeProps, output every 10 seconds
Resolves #13021

Auditors:

Test Plan:
…s more instantly

A buffer window is created at startup, and any time a previous buffer window is detached in order to be utilized as a real window.
This type of window is not shown and has no tabs (pinned or non-pinned) until it is shown.
State is not persisted for Buffer Windows until they are utilized as real windows.

A new command-line flag is introduced: '--debug-window-events' which shows logging around Buffer Window creation and utilization, as well as all other window events. No additional logging should be present without this flag.

Fix #12437
Needed for brave/https-everywhere-builder#3

Note that this is a breaking change from HTTPS Everywhere 5.x's ruleset
format.
@ryanml
Copy link
Contributor Author

ryanml commented Feb 19, 2018

See #13182

@ryanml ryanml closed this Feb 19, 2018
@NejcZdovc NejcZdovc removed this from the 0.23.x (Nightly Channel) milestone Feb 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants