Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[DDW-552] Line breaks on the address from exported PDF #2402

Conversation

topseniors
Copy link
Contributor

@topseniors topseniors commented Feb 25, 2021

This PR removed line break characters on the address from exported PDF
Jira Ticket

Todos

  • Remove line break characters on the address from exported PDF

Testing Checklist

App

  • Go to Wallet => Receive, click on Share button to open Address Share dialog, export address info to PDF by clicking on Save to PDF button, and make sure address from exported PDF doesn't include line break characters

Screenshots

English

image

Japanese

image

Test Cases

Scenario 1 - Review PDF Download
Given that I have a wallet loaded with transaction history
And I navigate to receive screen
And I click on share button
When I choose to download PDF and open it
Then I see the address does not include line break characters

Review Checklist

Basics

  • PR has been assigned and has appropriate labels (feature/bug/chore, release-x.x.x)
  • PR is updated to the most recent version of the target branch (and there are no conflicts)
  • PR has a good description that summarizes all changes and shows some screenshots or animated GIFs of important UI changes
  • CHANGELOG entry has been added to the top of the appropriate section (Features, Fixes, Chores) and is linked to the correct PR on GitHub
  • Automated tests: All acceptance and unit tests are passing (yarn test)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in development build (yarn dev)
  • Manual tests (minimum tests should cover newly added feature/fix): App works correctly in production build (yarn package / CI builds)
  • There are no flow errors or warnings (yarn flow:test)
  • There are no lint errors or warnings (yarn lint)
  • There are no prettier errors or warnings (yarn prettier:check)
  • There are no missing translations (running yarn manage:translations produces no changes)
  • Text changes are proofread and approved (Jane Wild / Amy Reeve)
  • Japanese text changes are proofread and approved (Junko Oda)
  • UI changes look good in all themes (Alexander Rukin)
  • Storybook works and no stories are broken (yarn storybook)
  • In case of dependency changes yarn.lock file is updated

Code Quality

  • Important parts of the code are properly commented and documented
  • Code is properly typed with flow
  • React components are split-up enough to avoid unnecessary re-renderings
  • Any code that only works in main process is neatly separated from components

Testing

  • New feature/change is covered by acceptance tests
  • New feature/change is manually tested and approved by QA team
  • All existing acceptance tests are still up-to-date
  • New feature/change is covered by Daedalus Testing scenario
  • All existing Daedalus Testing scenarios are still up-to-date

After Review

  • Merge the PR
  • Delete the source branch
  • Move the ticket to done column on the YouTrack board
  • Update Slack QA thread by marking it with a green checkmark

Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

@yakovkaravelov . Tested on 16779 on both Windows and Mac. Still not fixed. There's still a space, see the following (Note: the space is added between 5392 and k6ay. Also between hslc and lqek):
addr_test1qrgz43v6m8cc8ugxsnlv6ztnru229n5392 k6aya6sedgmc86x30xeux36hqv5uhm9qcr0hvf2hhslc lqekunu3h6re9scyqtmm

This is the PDF

@miorsufianiohk
Copy link

@miorsufianiohk I've just opened above pdf, but I can't find space when copying the address.

Hi @yakovkaravelov , the issue is on the Jira ticket.

Steps

Access to Wallet - Receive
Click on "Share"
Export the PDF file and open it
Copy and paste the address from the file
Obtained result
There are line breaks on the address, so when it's pasted there are spaces on the address.

Expected result
There should not be breaks or spaces on the address in the file.

@nikolaglumac
Copy link
Contributor

@yakovkaravelov @miorsufianiohk please get on a call and discuss this.

@nikolaglumac
Copy link
Contributor

@yakovkaravelov please merge latest develop.

Copy link
Contributor

@tomislavhoracek tomislavhoracek left a comment

Choose a reason for hiding this comment

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

@yakovkaravelov issue is still presented.
Try to copy the address from this doc and paste it somewhere. You will see spaces
Address-Ledger Nano S-2021-02-26T120718.0101.pdf

Screen Shot 2021-02-26 at 12 08 13

@nikolaglumac
Copy link
Contributor

nikolaglumac commented Mar 1, 2021

This is strange... 🤔
I don't get empty spaces even in the original version (develop build).
Perhaps this depends on the PDF reader more than on the way we create the PDF?

NOTE: This was tested on develop build!!!

Can you all try this one:
Address-Shelley Wallet-2021-03-01T102144.0251.pdf

My results:
Screenshot 2021-03-01 at 10 22 34

Browser:
Screenshot 2021-03-01 at 10 24 33

Text editor:
Screenshot 2021-03-01 at 10 24 15

☝️ I don't see a problem with this. In text editor case I get expected new lines while in the Browser's address bar I don't get any spaces...

@tomislavhoracek
Copy link
Contributor

@nikolaglumac hmmm weird. With your PDF, I have same results like you have. Without spaces.
Can you try PDF I posted?

@nikolaglumac
Copy link
Contributor

I get the same for your PDF too...
All looks well 🤔

Browser:
Screenshot 2021-03-01 at 10 38 28

Text editor:
Screenshot 2021-03-01 at 10 38 34

@tomislavhoracek
Copy link
Contributor

@nikolaglumac You are in right. It depends on PDF reader. If I open PDF in browser then there are no spaces but if I open with a reader installed on my machine then spaces are presented

@miorsufianiohk
Copy link

@nikolaglumac : I found no space observed only when I opened the PDF on Chrome browser and pasted the address onto Chrome URL text field. (see result below).

When I opened the PDF on Chrome, copied the address and pasted:
On Chrome URL text field: No space observed
On Safari URL text field: Space observed
On Daedalus->Wallet->Send_>Receiver text field: Space observed

When I opened the PDF on Safari, copied the address and pasted:
On Chrome URL text field: Space observed
On Safari URL text field: Space observed
On Daedalus->Wallet->Send_>Receiver text field: Space observed

When I opened the PDF Reader Pro Lite copied the address and pasted:
On Chrome URL text field: Space observed
On Safari URL text field: Space observed
On Daedalus->Wallet->Send_>Receiver text field: Space observed

@topseniors
Copy link
Contributor Author

@nikolaglumac : I found no space observed only when I opened the PDF on Chrome browser and pasted the address onto Chrome URL text field. (see result below).

When I opened the PDF on Chrome, copied the address and pasted:
On Chrome URL text field: No space observed
On Safari URL text field: Space observed
On Daedalus->Wallet->Send_>Receiver text field: Space observed

When I opened the PDF on Safari, copied the address and pasted:
On Chrome URL text field: Space observed
On Safari URL text field: Space observed
On Daedalus->Wallet->Send_>Receiver text field: Space observed

When I opened the PDF Reader Pro Lite copied the address and pasted:
On Chrome URL text field: Space observed
On Safari URL text field: Space observed
On Daedalus->Wallet->Send_>Receiver text field: Space observed

@miorsufianiohk
If you use this branch build, white space will not appear in more cases, but one thing clear is that pasting in receiver field always shows white space.
Seems like it is related to \r\n and \n characters added.
I was thinking about trying other npm package to write pdf, just wanted to get permission before trying it.
CC @nikolaglumac @darko-mijic

@nikolaglumac
Copy link
Contributor

Perhaps we need to think about stripping whitespace in the receiver input on the send form?
cc @darko-mijic

@topseniors
Copy link
Contributor Author

topseniors commented Mar 2, 2021

@nikolaglumac @tomislavhoracek @gabriela-ponce @miorsufianiohk @ManusMcCole @alexander-rukin
I applied small font to the address. Please review/test, thanks.

@alexander-rukin
Copy link
Contributor

alexander-rukin commented Mar 2, 2021

image

these fields (description and network) should have smaller fonts too - https://zpl.io/blMomG5

with size 9px for address 12px size is more than enought for these elements

Copy link

@miorsufianiohk miorsufianiohk left a comment

Choose a reason for hiding this comment

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

Line breaks issue no longer exist when tested on smaller font with continuous one line address. Great work @yakovkaravelov . 👍 . Tested on 16891

Copy link
Contributor

@alexander-rukin alexander-rukin left a comment

Choose a reason for hiding this comment

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

Pls add styles to the description and network parts

@topseniors
Copy link
Contributor Author

@alexander-rukin Would you please check this out?
Thanks.

@topseniors topseniors removed the WIP label Mar 4, 2021
@alexander-rukin
Copy link
Contributor

alexander-rukin commented Mar 4, 2021

is this the same?

image

and

image

live fonts seem bigger 🤔

UPD: font sizes are correct (pdf is just scaling on a screen and it makes feel you something is different)
We are missing some styles from the original design. If it's hard to implement (we faced some issue last time if I remember correctly) we can just go with this version.

@nikolaglumac nikolaglumac merged commit ae24ad9 into develop Mar 4, 2021
@iohk-bors iohk-bors bot deleted the chore/ddw-552-line-breaks-on-the-address-from-exported-pdf branch March 4, 2021 17:07
@nikolaglumac nikolaglumac added release-4.0.2-FC3 Daedalus Flight and removed ⏳release-vNext labels Mar 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants