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

Adds server problem overlay #13973

Merged
merged 1 commit into from
May 1, 2018
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Apr 30, 2018

Resolves #13972

New look:
image

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:

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

@NejcZdovc NejcZdovc added this to the 0.22.x Release 3 (Beta channel) milestone Apr 30, 2018
@NejcZdovc NejcZdovc self-assigned this Apr 30, 2018
Resolves brave#13972

Auditors:

Test Plan:
@codecov-io
Copy link

codecov-io commented Apr 30, 2018

Codecov Report

Merging #13973 into master will decrease coverage by 0.06%.
The diff coverage is 26.08%.

@@            Coverage Diff             @@
##           master   #13973      +/-   ##
==========================================
- Coverage   56.55%   56.49%   -0.07%     
==========================================
  Files         284      284              
  Lines       29280    29305      +25     
  Branches     4865     4869       +4     
==========================================
- Hits        16559    16555       -4     
- Misses      12721    12750      +29
Flag Coverage Δ
#unittest 56.49% <26.08%> (-0.07%) ⬇️
Impacted Files Coverage Δ
app/locale.js 35.77% <ø> (ø) ⬆️
js/constants/appConstants.js 100% <ø> (ø) ⬆️
js/actions/appActions.js 19.16% <0%> (-0.11%) ⬇️
...r/components/preferences/payment/enabledContent.js 64.76% <0%> (-1.9%) ⬇️
app/common/constants/ledgerStatuses.js 100% <100%> (ø) ⬆️
app/browser/api/ledger.js 63.07% <25%> (-0.08%) ⬇️
app/browser/reducers/ledgerReducer.js 46.24% <75%> (-0.44%) ⬇️
js/stores/appStoreRenderer.js 91.66% <0%> (-8.34%) ⬇️
app/renderer/components/reduxComponent.js 57.75% <0%> (-3.45%) ⬇️
... and 3 more

@jasonrsadler
Copy link
Contributor

jasonrsadler commented Apr 30, 2018

Code looks good. ~~~I would change ledgerStatuses to just ledgerStatus unless we're returning multiple statuses. Other than that,~~~ I'm waiting for functionality testing.

Was thinking that was a new file

Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

Can we change the min-height under the selector enabledContent__overlay from 159px to 188px?

Due to the amount of content in the error overlay, it leaves an odd gap exposed at the bottom, compared to the corrupted wallet overlay, which fits the height of the underlying container with wallet information.

Compare:

screen shot 2018-04-30 at 9 26 01 am

With

screen shot 2018-04-30 at 9 27 57 am

Edit: This may not be an issue once the copy changes.

@mrose17 mrose17 self-requested a review April 30, 2018 22:54
Copy link
Member

@mrose17 mrose17 left a comment

Choose a reason for hiding this comment

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

Approved.

@jasonrsadler jasonrsadler self-requested a review May 1, 2018 00:44
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

Since there is a v.Next upcoming, I would say any cosmetic or convenience issues can be addressed then. @ryanml wdyt?

Copy link
Contributor

@ryanml ryanml left a comment

Choose a reason for hiding this comment

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

@jasonrsadler sgtm, approved

@jasonrsadler jasonrsadler merged commit 7cc0aef into brave:master May 1, 2018
@bsclifton
Copy link
Member

@jasonrsadler since this will go to 0.22.x release 3, don't forget to merge it to:
0.23.x
0.22.x-release3

NejcZdovc pushed a commit that referenced this pull request May 1, 2018
NejcZdovc pushed a commit that referenced this pull request May 1, 2018
@NejcZdovc
Copy link
Contributor Author

master 7cc0aef
0.23 084f6e1
0.22.x-release3 cea9a88

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.

6 participants