UI for Rewards monthly statement #513

jasonrsadler · 2019-07-09T21:40:14Z

Core: brave/brave-core#2733

Changes

Implements UI portions for Rewards Monthly Statement for brave/brave-browser#930

Test plan

Link / storybook path to visual changes

https://brave-ui-3c5q3df9x.now.sh/?path=/story/feature-components-rewards-modal--monthly-statement

https://brave-ui-3c5q3df9x.now.sh/?path=/story/feature-components-rewards-concepts-desktop--printable-monthly-statement

Integration

Does this contain changes to src/components or src/
- Will you publish to npm immediately after this PR, or wait until sometime in the future?
- Incompatible API change to something existing (major version increase)
- Adding new backwards-compatible functionality? (minor version increase)
- Fixing a bug backwards-compatibly? (patch version increase)
Does this contain changes to src/features for brave-core?
- Are there non backwards-compatible changes required for brave-core? Do not merge until brave-core PR is approvable. Link to brave-core PR:
- Will you create brave-core PR to update to this commit after it is merged?
- Wants uplift to brave-core feature branch?
  - When uplift-approved, merge to brave-core-0.VV.x feature branch
  - Create additional brave-core PRs for each feature branch to update commit

rossmoody · 2019-07-10T19:29:04Z

I think this one might need an up to date design spec attached to it, right? @jenn-rhim I see the one in the issue but this PR seems to have tabbed navigation within the modal and is missing a few of the UI elements like Download as PDF and opening/closing balance line items. I think it might be tough for QA to know what's right without it.

Once we have that it'd be good to tighten up a few style nits like the floating divider line.

rossmoody · 2019-07-10T21:17:55Z

Issue updated with design. Thanks for pointing me in the right direction Jason.

jasonrsadler · 2019-07-11T01:02:53Z

@rossmoody
Download as PDF and Opening/Closing balances are deferred and are being handled in subsequent PRs.

Overflow problem for shrinking window is fixed.

NejcZdovc · 2019-07-11T13:22:57Z

src/components/dataTables/table/index.tsx

@@ -21,17 +21,18 @@ export interface Props {
  children?: React.ReactNode
  rows?: Row[]
  rowTheme?: {[key: string]: string}
+  customStyle?: {[key: string]: string}


cc @petemill and @cezaraugusto as it changes common component

After conferring with @cezaraugusto and @petemill, removed modifications to table and tabs component

NejcZdovc · 2019-07-11T13:24:01Z

src/components/layout/tabs/style.ts

  text-align: center;
  font-family: ${p => p.theme.fontFamily.body};
+  height: 60px;


cc @petemill and @cezaraugusto as it changes common component

After conferring with @cezaraugusto and @petemill, removed modifications to table and tabs component

NejcZdovc · 2019-07-11T13:28:25Z

src/features/rewards/modalActivity/index.tsx

@@ -58,6 +58,8 @@ export interface SummaryItem {

 export interface Props {
  contributeRows: ContributeRow[]
+  donationRows: ContributeRow[]


we don't use donation phrase anymore. We only have AC, tip and recurring tip

NejcZdovc · 2019-07-11T13:30:07Z

src/features/rewards/modalActivity/index.tsx

-            </StyledVerified>
+            <div>
+              <Tabs activeTabId={this.state.tabId} onChange={this.onChange}>
+                <div data-key='cont1' data-title={'Transactions'}>


translation needed

NejcZdovc · 2019-07-11T13:30:11Z

src/features/rewards/modalActivity/index.tsx

+                    {getLocale('noStatementTransactions')}
+                  </TableTransactions>
+                </div>
+                <div data-key='cont2' data-title={'Monthly contributions'}>


translation needed

NejcZdovc · 2019-07-11T13:30:16Z

src/features/rewards/modalActivity/index.tsx

+                    {getLocale('paymentMonthly', { day: paymentDay })}
+                  </StyledTableSubTitle>
+                </div>
+                <div data-key='cont3' data-title={'Auto-Contribute'}>


translation needed

NejcZdovc · 2019-07-11T13:30:30Z

src/features/rewards/modalActivity/index.tsx

+                    {getLocale('paymentMonthly', { day: paymentDay })}
+                  </StyledTableSubTitle>
+                </div>
+                <div data-key='cont4' data-title={'Tips'}>


translation needed

NejcZdovc · 2019-07-11T13:31:12Z

src/features/rewards/modalActivity/index.tsx

-              </StyledVerifiedIcon>
-              <StyledVerifiedText>{getLocale('braveVerified')}</StyledVerifiedText>
-            </StyledVerified>
+            <div>


is this div needed?

NejcZdovc · 2019-07-11T13:34:10Z

src/features/rewards/printableActivity/style.ts

+
+export const StyledTitle = styled<{}, 'div'>('div')`
+  font-weight: 600;
+  color: #1B1D2F;


please use theme color

NejcZdovc · 2019-07-11T13:34:14Z

src/features/rewards/printableActivity/style.ts

+`
+
+export const StyledSubTitle = styled<{}, 'span'>('span')`
+  color: #838391;


please use theme color

NejcZdovc · 2019-07-11T13:34:19Z

src/features/rewards/printableActivity/style.ts

+export const StyledSelectOption = styled<{}, 'div'>('div')`
+  font-size: 22px;
+  font-weight: 300;
+  color: #4C54D2;


please use theme color

NejcZdovc · 2019-07-11T13:35:20Z

src/features/rewards/printableActivity/style.ts

+export const StyledIconText = styled<{}, 'div'>('div')`
+  font-size: 14px;
+  line-height: 1.43;
+  color: #838391;


please use theme color

NejcZdovc · 2019-07-11T13:35:31Z

src/features/rewards/printableActivity/style.ts

+export const StyledWarning = styled<{}, 'div'>('div')`
+  display: flex;
+  justify-content: center;
+  border-top: 1px solid #ebecf0;


please use theme color

++ If there is not a generic and shareabel theme color, then we can create one after asking design team @jenn-rhim @rossmoody, e.g. warningBackgroundBorder.

If this is a rewards-specific color rule then maybe we should start a rewards theme.

NejcZdovc · 2019-07-11T13:36:02Z

src/features/rewards/printableActivity/style.ts

+  font-weight: 300;
+  line-height: 2.79;
+  letter-spacing: 0.2px;
+  color: #4b4c5c;


please use theme color

NejcZdovc · 2019-07-11T13:36:07Z

src/features/rewards/printableActivity/style.ts

+  font-size: 12px;
+  align-items: center;
+  line-height: 2;
+  color: #9e9fab;


please use theme color

NejcZdovc · 2019-07-11T13:36:12Z

src/features/rewards/printableActivity/style.ts

+`
+
+export const StyledAlertWrapper = styled<{}, 'div'>('div')`
+  color: #E9AB18;


please use theme color

NejcZdovc · 2019-07-11T13:36:14Z

src/features/rewards/printableActivity/style.ts

+`
+
+export const StyledActionIcon = styled<{}, 'span'>('span')`
+  color: #A1A8F2;


please use theme color

NejcZdovc · 2019-07-11T13:36:19Z

src/features/rewards/printableActivity/style.ts

+
+export const StyledVerifiedIcon = styled<{}, 'div'>('div')`
+  display: flex;
+  color: #392DD1;


please use theme color

NejcZdovc · 2019-07-11T13:42:59Z

@jasonrsadler which version is correct? There is a lot of differences.

left is yours on the right is what we have in the abstract

jasonrsadler · 2019-07-11T17:21:29Z

@NejcZdovc
This is the spec I received as final (and is named as final). @rossmoody, which one is considered final?

jasonrsadler · 2019-07-11T18:05:13Z

@NejcZdovc Re: theme colors and fonts above. These are the colors that match the spec as well as fonts to match as closely as possible. We don't currently have theme colors to match these and would propose a follow up be done to streamline these.

rossmoody · 2019-07-11T18:07:01Z

It's worth it to have @jenn-rhim weigh in on this one. I'm not close enough to this to know what is or isn't correct. I found that mockup in Abstract but if we've been working off a supplied design it's probably safe to assume that's the right version. I'll update the issue.

NejcZdovc · 2019-07-11T20:57:31Z

@jasonrsadler colors should be picked from theme pallet, so let's try to find the closest color to what it is picked in the design

NejcZdovc · 2019-07-11T20:58:17Z

@jasonrsadler what you pasted and what I did in my comparison is almost the same, the only thing that is different is footer text wrapping

jasonrsadler · 2019-07-11T21:23:18Z

@NejcZdovc regardless, they are different spec files in abstract and I didn’t recognize that file. Again, if we’re adhering to the spec, these are the correct colors. We should define and themify these as appropriate outside of this PR.

cezaraugusto

looks good to me. code looks good and interface is aligned with https://share.goabstract.com/a6b555b0-5f5f-4c0e-8ae8-7d85d31eb7e6?mode=design&sha=59c3c2ccee892fdec04a35a8db19514246401e92

petemill

Would be good to have a better way to override Table margin than wrap with negative margin.

Then for colors as @NejcZdovc stated, please use theme colors so we don't dig the whole deeper of difficulty to create dark theme.

petemill · 2019-07-15T19:19:32Z

src/features/rewards/modalActivity/style.ts

+`
+
+export const StyledTable = styled<{}, 'div'>('div')`
+  margin: -24px 0;


This will cause an overlap if the default margin changes of the Table component changes. Can we simply call something like?

styled(Table)` margin: 0; `

The whole situation isn't ideal, but somehow resetting it to 0 is better than negative-offsetting the currently-set margin value.

petemill · 2019-07-15T19:19:54Z

src/features/rewards/modalActivity/style.ts

+  text-align: center;
+  font-family: ${p => p.theme.fontFamily.body};
+  height: 60px;
+  padding-top:5px;


space between property and value

petemill · 2019-07-15T19:22:31Z

src/features/rewards/printableActivity/style.ts

+export const StyledWarning = styled<{}, 'div'>('div')`
+  display: flex;
+  justify-content: center;
+  border-top: 1px solid #ebecf0;


++ If there is not a generic and shareabel theme color, then we can create one after asking design team @jenn-rhim @rossmoody, e.g. warningBackgroundBorder.

If this is a rewards-specific color rule then maybe we should start a rewards theme.

petemill · 2019-07-15T19:23:18Z

src/features/rewards/tableTransactions/style.ts

+  contributeTx: '#9752cb',
+  recurringDonationTx: '#696fdc',
+  grantsTx: '#c12d7c',
+  earningFromAdsTx: '#c12d7c'


Theme colors

NejcZdovc

Can you please remove this check as I don't see it in abstract

Icon is missing on the second tab

Icon missing on the last tab

Can we remove jumping UI when you switch between tabs. (not talking about modal height, but on text moving left and right; see summary table at the top)

This needs to be changed to Total until we support start/end month

We are missing month in printable version

jasonrsadler added the rewards label Jul 9, 2019

jasonrsadler requested review from ryanml, NejcZdovc and rossmoody July 9, 2019 21:40

jasonrsadler self-assigned this Jul 9, 2019

jasonrsadler force-pushed the monthly-statement branch from 39ffe98 to 3482c89 Compare July 9, 2019 21:48

jasonrsadler added the PR/pending-review label Jul 9, 2019

jasonrsadler force-pushed the monthly-statement branch 2 times, most recently from 3064edb to 07c876b Compare July 10, 2019 18:22

jasonrsadler force-pushed the monthly-statement branch from 07c876b to f259bb7 Compare July 10, 2019 19:55

jasonrsadler mentioned this pull request Jul 10, 2019

Monthly statement can now be viewed from Rewards page brave/brave-core#2733

Closed

32 tasks

jasonrsadler force-pushed the monthly-statement branch 2 times, most recently from 2c4ea97 to 3eafa18 Compare July 11, 2019 01:01

jasonrsadler force-pushed the monthly-statement branch from 3eafa18 to b72c24e Compare July 11, 2019 01:11

NejcZdovc reviewed Jul 11, 2019

View reviewed changes

NejcZdovc requested review from cezaraugusto, NejcZdovc and petemill July 11, 2019 13:54

jasonrsadler force-pushed the monthly-statement branch from b72c24e to 60e7360 Compare July 11, 2019 15:39

jasonrsadler force-pushed the monthly-statement branch 8 times, most recently from 62d3320 to 4275145 Compare July 12, 2019 12:06

monthly statement

7031b1f

jasonrsadler force-pushed the monthly-statement branch from 4275145 to 7031b1f Compare July 12, 2019 15:07

cezaraugusto approved these changes Jul 15, 2019

View reviewed changes

petemill suggested changes Jul 15, 2019

View reviewed changes

NejcZdovc suggested changes Jul 15, 2019

View reviewed changes

ryanml closed this Jul 25, 2019

UI for Rewards monthly statement #513

UI for Rewards monthly statement #513

Conversation

jasonrsadler commented Jul 9, 2019 • edited Loading

Changes

Test plan

Link / storybook path to visual changes

Integration

rossmoody commented Jul 10, 2019

rossmoody commented Jul 10, 2019

jasonrsadler commented Jul 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc commented Jul 11, 2019

jasonrsadler commented Jul 11, 2019 • edited Loading

jasonrsadler commented Jul 11, 2019

rossmoody commented Jul 11, 2019

NejcZdovc commented Jul 11, 2019

NejcZdovc commented Jul 11, 2019

jasonrsadler commented Jul 11, 2019 • edited Loading

cezaraugusto left a comment

Choose a reason for hiding this comment

petemill left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NejcZdovc left a comment • edited Loading

Choose a reason for hiding this comment

jasonrsadler commented Jul 9, 2019 •

edited

Loading

jasonrsadler commented Jul 11, 2019 •

edited

Loading

jasonrsadler commented Jul 11, 2019 •

edited

Loading

NejcZdovc left a comment •

edited

Loading