Refactor contributionStatement.js with Aphrodite #7887

luixxiul · 2017-03-25T10:37:16Z

Remove contributionStatement.less
Replace inlined braveLogo with a PNG file, verifiedIcon with a SVG file
Introduce constants for margins, borders, paddings, light gray, etc

Auditors:

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).

Test Plan:

npm run add-simulated-payment-history
Open about:preferences#payments
Display the payments history
Save the PDF file
Open it and make sure it is successfully rendered

luixxiul · 2017-03-25T10:50:46Z

The brave logo PNG file and the verified SVG icon are not displayed on PDF file. Could I know how to display them? Do I have to inline them?

willy-b

Hey @luixxiul,
This looks great. I see two things.

(1) Inline images are required for PDF generation, I think.
At least the final rendered HTML will need images inlined as data URIs for a PDF to be generated properly with existing code. You may be able to keep the data URI in a separate file and require it as you have done with the images here, though.

(2) Page breaks were lost with this change, need 1 line added to CSS (pageBreakAfter: 'always' incontributionStatement__page), see below

willy-b · 2017-04-01T09:37:05Z

js/about/contributionStatement.js

+  // ContributionStatementPage
+  contributionStatement__page: {
+    display: 'flex',
+    flexFlow: 'column nowrap'


Should add pageBreakAfter: 'always' here in contributionStatement__page so that there is a page break in PDF between pages.

Current code:

With pageBreakAfter: 'always':

Thanks for your feedback @willy-b! I'll ping you again later.

Added here: 2f16bbd#diff-dfc3cf3000430cbd1575b0223b06163fR630

I think you fixed this in next commit which was force pushed over this and the previous one, but pageBrakeAfter here (2f16bbd#diff-dfc3cf3000430cbd1575b0223b06163fR630) should be replaced with pageBreakAfter

Silly mistake.. I updated here: 0164a4d#diff-dfc3cf3000430cbd1575b0223b06163fR630

luixxiul · 2017-04-01T17:17:59Z

@willy-b Updated. The files were not displayed because npm run watch had been killed. I confirmed it worked as it was.

willy-b · 2017-04-01T23:19:31Z

Glad it's working for you now. One issue: I still don't see the images in the generated PDF with your latest revision.

The footer text also continues to have extra characters in it, which seem to be have been fixed in your latest screenshot. Are you sure that screenshot is of 0164a4d?

luixxiul · 2017-04-02T04:19:00Z

Would you try rm -rf ~/.electron && rm -rf node_modules and npm i? Maybe something has been fixed.

willy-b · 2017-04-02T05:35:25Z

Sure. I just re-installed: Still don't see any images in the generated PDF (which I can attach if you like).
Though I can confirm that page-breaking is fixed (#7887 (comment))

luixxiul · 2017-04-02T06:51:46Z

I'm not sure where the issue is...

create payment history
npm run watch
npm start
open about:preferences#payments
show payment history
click a pdf
preview it

willy-b · 2017-04-02T07:28:39Z

It looks like you're using the Print dialog manually to print to PDF and open the PDF in preview? That actually works for me too.

But what about the auto-popup for saving the PDF that is triggered by current code? That does not work (it uses a data URI internally and so the generated PDF lacks images). (Did you turn this off in your local for making the screencapture?) We could replace that with asking user to use Print -> Print to PDF and choose their own filename.

luixxiul · 2017-04-02T07:34:27Z

the auto-popup for saving the PDF that is triggered by current code?

Actually I haven't seen it for a while, though I'm running on the latest code. Is it disabled??

willy-b · 2017-04-02T08:36:39Z

I see the auto-popup save dialog in a fresh build from your commit 0164a4d :

As you can see in my screencap, the images are missing from the downloaded PDF (opened from downloads bar).

I wonder why you're not seeing the Download PDF auto-popup dialog anymore?

If I do a manual File->Print->Print to PDF of the contribution statement HTML page, as you show in your screencap, then the images are preserved for me too.
We could get rid of the auto-popup and just tell people to do that if you want.

luixxiul · 2017-04-02T10:22:47Z

I'm on macOS. Is it related with Muon or something? / CC @darkdh @bsclifton

darkdh · 2017-04-02T16:43:32Z

I think it related to our 57 upgrade. There will be PREVIEW_FAIL error on both windows and macOS.

luixxiul · 2017-04-02T17:13:11Z

PREVIEW_FAIL

Yeah I see it on macOS!

luixxiul · 2017-04-03T14:24:31Z

@willy-b For me, not displaying the pop up is not that bad; in fact I felt quite natural, since I was not forced to save the statement in PDF. I could save, print, copy or just read through the statement of my own will.

I could imagine some would say, "please do not display the save pop up window as it is annoying."

bsclifton · 2017-04-03T18:13:43Z

@luixxiul is this ready for review? or are you still working out a few things?

luixxiul · 2017-04-03T18:41:06Z

Except inlining the PNG and SVG files, it's ready for review.

If it is impossible to add the external files with printToPDF, I would inline them. / CC @darkdh

luixxiul · 2017-04-05T18:22:10Z

@willy-b would you please work on the fix? thanks!

cezaraugusto · 2017-04-05T18:22:48Z

@willy-b sorry I missed your last comment before merging. Please feel free to open an issue, not sure which STR to repro error you mentioned

luixxiul · 2017-04-05T18:23:27Z

@cezaraugusto @willy-b ^ #7887 (comment)

willy-b · 2017-04-05T18:26:02Z

no worries @cezaraugusto . quick question: does the final version you merged use inline images or not?

cezaraugusto · 2017-04-05T18:46:04Z

no inline-images.

A fix I can think of is to convert images to base64 before rendering, but feel free to apply any method you find best and lmk if you need anything. Thanks for following-up :)

cezaraugusto · 2017-04-05T18:46:42Z

btw auto-popup doesn't show on macOS

luixxiul · 2017-04-05T18:56:14Z

it has been mentioned here: #7887 (comment)

willy-b · 2017-04-05T18:56:36Z

okay so we just broke this, just so we could avoid inline images? fair enough.
still seems to me we should have done that in a separate PR.

@darkdh is fixing the popup on MacOS, I think.

darkdh · 2017-04-05T19:54:54Z

It's been fixed in brave/muon@9d32f7f
Just needs a new muon build

willy-b · 2017-04-05T20:01:43Z

awesome @darkdh.

so then the popup will be on MacOS and images will be broken in the PDF. you guys sure you don't want to merge @luixxiul's other commit which restores inline images temporarily so this is not broken?

cezaraugusto · 2017-04-05T20:16:03Z

@willy-b I'm looking for an alternative but yes we'll end up inlining images just not very happy with hard-coding it. If time gets shorter and we can't make it we'll eventually go back to @luixxiul fix.

willy-b · 2017-04-05T20:27:53Z

ok, so you're going to restore inline images and reopen #8062 to pull them out once we make them unnecessary?

darkdh · 2017-04-05T20:45:06Z

@willy-b , I think image problem will also be fixed in that commit

willy-b · 2017-04-05T20:58:38Z

unfortunately it won't. the image issue is not in Muon. it's in our use of a data URI here: 7c32ef1#diff-dfc3cf3000430cbd1575b0223b06163fR120

luixxiul added feature/rewards refactoring/aphrodite labels Mar 25, 2017

luixxiul added this to the 0.14.1 milestone Mar 25, 2017

luixxiul requested review from cezaraugusto, bsclifton and NejcZdovc March 25, 2017 10:37

luixxiul added the PR/needs-QA-attention ☕ label Mar 25, 2017

luixxiul added the help wanted The PR/issue opener needs help to complete/report the task. label Mar 25, 2017

luixxiul requested a review from willy-b March 25, 2017 12:44

luixxiul self-assigned this Mar 27, 2017

willy-b suggested changes Apr 1, 2017

View reviewed changes

luixxiul removed the help wanted The PR/issue opener needs help to complete/report the task. label Apr 1, 2017

luixxiul mentioned this pull request Apr 3, 2017

printToPDF issue of chromium 57 #8051

Closed

mrose17 mentioned this pull request Apr 3, 2017

Added current year in PDF statement copyright footer #6878

Closed

luixxiul added the PR/work-in-progress ⚒ label Apr 3, 2017

cezaraugusto merged commit 7c32ef1 into brave:master Apr 5, 2017

luixxiul deleted the contributionStatement-aphrodite branch April 5, 2017 18:22

luixxiul removed the PR/needs-QA-attention ☕ label Apr 5, 2017

cezaraugusto mentioned this pull request Apr 6, 2017

Make use of base64 for contributionStatement img #8107

Merged

luixxiul removed the request for review from bsclifton April 8, 2017 08:39

This was referenced Apr 21, 2017

Manual test run for Ubuntu 0.15.0 RC3 #8430

Closed

Manual test run for Windows x64 0.15.0 RC3 #8431

Closed

Manual tests for OS X 0.15.0 RC3 #8433

Closed

Manual test OS X per release items for 0.15.0 RC5 #8492

Closed

Refactor contributionStatement.js with Aphrodite #7887

Refactor contributionStatement.js with Aphrodite #7887

Conversation

luixxiul commented Mar 25, 2017 • edited Loading

luixxiul commented Mar 25, 2017 • edited Loading

willy-b left a comment • edited Loading

Choose a reason for hiding this comment

willy-b Apr 1, 2017

Choose a reason for hiding this comment

luixxiul Apr 1, 2017

Choose a reason for hiding this comment

luixxiul Apr 1, 2017

Choose a reason for hiding this comment

willy-b Apr 1, 2017

Choose a reason for hiding this comment

luixxiul Apr 1, 2017

Choose a reason for hiding this comment

luixxiul commented Apr 1, 2017 • edited Loading

willy-b commented Apr 1, 2017 • edited Loading

luixxiul commented Apr 2, 2017 • edited Loading

willy-b commented Apr 2, 2017 • edited Loading

luixxiul commented Apr 2, 2017

willy-b commented Apr 2, 2017 • edited Loading

luixxiul commented Apr 2, 2017

willy-b commented Apr 2, 2017

luixxiul commented Apr 2, 2017

darkdh commented Apr 2, 2017

luixxiul commented Apr 2, 2017

luixxiul commented Apr 3, 2017

bsclifton commented Apr 3, 2017

luixxiul commented Apr 3, 2017

luixxiul commented Apr 5, 2017

cezaraugusto commented Apr 5, 2017

luixxiul commented Apr 5, 2017

willy-b commented Apr 5, 2017

cezaraugusto commented Apr 5, 2017

cezaraugusto commented Apr 5, 2017

luixxiul commented Apr 5, 2017

willy-b commented Apr 5, 2017

darkdh commented Apr 5, 2017

willy-b commented Apr 5, 2017

cezaraugusto commented Apr 5, 2017

willy-b commented Apr 5, 2017

darkdh commented Apr 5, 2017

willy-b commented Apr 5, 2017

luixxiul commented Mar 25, 2017 •

edited

Loading

luixxiul commented Mar 25, 2017 •

edited

Loading

willy-b left a comment •

edited

Loading

luixxiul commented Apr 1, 2017 •

edited

Loading

willy-b commented Apr 1, 2017 •

edited

Loading

luixxiul commented Apr 2, 2017 •

edited

Loading

willy-b commented Apr 2, 2017 •

edited

Loading

willy-b commented Apr 2, 2017 •

edited

Loading