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

🎨 [Guide Re-org] - Consolidated Onboarding and Payments into the Daily Spending Wallet reference design #739

Merged
merged 11 commits into from
Apr 5, 2022

Conversation

Bosch-0
Copy link
Collaborator

@Bosch-0 Bosch-0 commented Apr 4, 2022

Consolidated Onboarding and Payments into a Daily spending wallet reference design section.

Closes #718
Closes #721

💻 Deploy Preview

@Bosch-0 Bosch-0 requested review from GBKS, pavlenex, sbddesign and danielnordh and removed request for GBKS and pavlenex April 4, 2022 13:12
@pavlenex
Copy link
Contributor

pavlenex commented Apr 4, 2022

Screenshot 2022-04-04 at 18 08 28

🤯 This one is going to take a while to review. I assume this is because of all the link changes.

Copy link
Contributor

@pavlenex pavlenex left a comment

Choose a reason for hiding this comment

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

@Bosch-0 Since I'm not sure what was the consensus regarding the implementation, I only reviewed for broken links.

Rake test found 18 broken links. Unfortunatelly do to large amount of changes, my GitHub web crashes, so I cannot recommend them directly, instead here's the log of broken internal links that need to be fixed, luckily the log is quite descriptive on which pages have broken links.



Ran on 104 files!


- ./_site/guide/case-studies/cloud-backup/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 347)
     <a href="/guide/onboarding/introduction/">Onboarding</a>
- ./_site/guide/case-studies/savings-account/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 316)
     <a href="/guide/onboarding/introduction/">onboarding</a>
- ./_site/guide/case-studies/shared-account/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 337)
     <a href="/guide/onboarding/introduction/">onboarding</a>
- ./_site/guide/case-studies/upgradeable-wallet/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 311)
     <a href="/guide/onboarding/introduction/">onboarding</a>
- ./_site/guide/daily-spending-wallet/backup-and-recovery/manual-backup/index.html
  *  internally linking to /daily-spending-wallet/backup-and-recovery/recovery/, which does not exist (line 688)
     <a class="next-previous--next" href="/daily-spending-wallet/backup-and-recovery/recovery/" rel="next">
      Recovery
      <svg viewBox="0 0 24 24">
        <use xlink:href="#svg-arrow-right"></use>
      </svg>
    </a>
- ./_site/guide/daily-spending-wallet/backup-and-recovery/recovery/index.html
  *  internally linking to /guide/daily-spending-wallet/request, which does not exist (line 366)
     <a href="/guide/daily-spending-wallet/request">requesting</a>
- ./_site/guide/daily-spending-wallet/first-use/index.html
  *  internally linking to /guide/daily-spending-wallet//restoring-a-wallet/, which does not exist (line 295)
     <a href="/guide/daily-spending-wallet//restoring-a-wallet/">Restore an existing wallet</a>
  *  internally linking to /guide/daily-spending-wallet/backing-up-a-wallet/, which does not exist (line 296)
     <a href="/guide/daily-spending-wallet/backing-up-a-wallet/">Backup a wallet</a>
  *  internally linking to /guide/daily-spending-wallet/funding-a-wallet/, which does not exist (line 297)
     <a href="/guide/daily-spending-wallet/funding-a-wallet/">Fund a wallet</a>
- ./_site/guide/daily-spending-wallet/funding/index.html
  *  internally linking to /guide/daily-spending-wallet/backup-and-recovery/, which does not exist (line 754)
     <a href="/guide/daily-spending-wallet/backup-and-recovery/">backups &amp; recovery</a>
  *  internally linking to /guide/daily-spending-wallet/backup-and-recovery/, which does not exist (line 760)
     <a class="next-previous--next" href="/guide/daily-spending-wallet/backup-and-recovery/" rel="next">
      Backing up a wallet
      <svg viewBox="0 0 24 24">
        <use xlink:href="#svg-arrow-right"></use>
      </svg>
    </a>
- ./_site/guide/daily-spending-wallet/requesting/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 545)
     <a href="/guide/onboarding/introduction/">onboarding</a>
- ./_site/guide/designing-products/common-user-flows/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 572)
     <a href="/guide/onboarding/introduction/">onboarding section</a>
- ./_site/guide/designing-products/usage-life-cycle/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 392)
     <a href="/guide/onboarding/introduction/">onboarding experience</a>
- ./_site/guide/designing-products/wallet-interoperability/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 528)
     <a href="/guide/onboarding/introduction/">onboarding</a>
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 534)
     <a class="next-previous--next" href="/guide/onboarding/introduction/" rel="next">
      Onboarding
      <svg viewBox="0 0 24 24">
        <use xlink:href="#svg-arrow-right"></use>
      </svg>
    </a>
- ./_site/guide/getting-started/principles/index.html
  *  internally linking to /guide/onboarding/introduction/, which does not exist (line 378)
     <a href="/guide/onboarding/introduction/">onboarding</a>
- ./_site/guide/glossary/address/index.html
  *  internal image /assets/images/guide/payments/request/[email protected] does not exist (line 383)

HTML-Proofer found 18 failures!

Copy link
Collaborator

@sbddesign sbddesign left a comment

Choose a reason for hiding this comment

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

Looking pretty good. cACK on bundling recovery with the backups in the nav.

Requesting changes:

  1. There are two pages footers with outdated descriptions for the next link, left inline comments to point these out.
  2. rake test has 18 broken internal links, same ones @pavlenex indicated.
  3. Daily spending image for intro page would be good, but could be handled in another PR.

Fix 1 and 2 and I'll review it again.

guide/daily-spending-wallet/privacy.md Outdated Show resolved Hide resolved
@Bosch-0
Copy link
Collaborator Author

Bosch-0 commented Apr 5, 2022

Fixed all the above comments, also moved content from https://bitcoin.design/guide/case-studies/cloud-backup/ to this new section and removed that case study. We can re-purpose some of that content when we make a reference design page inside the daily spending wallet section (brings all the chapter together into a prototype).

Copy link
Contributor

@GBKS GBKS left a comment

Choose a reason for hiding this comment

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

Huge and tedious update, thanks for working through this. Had some minor comments. If we want to move forward with merging this PR, I'm totally fine with handling text changes in a separate one.

For future re-orgs like these, let's split things up a bit further. The Github review page was so overloaded that my browser didn't work properly until I managed to use the file filter to only show markdown files. Moving images can happen in separate PRs (maybe even page-by-page to split it up further).

My review included:

  • Running the site locally
  • rake test
  • Going over all markdown files on Github
  • Clicking through all pages locally and checking Terminal and Browser Dev Tools for issues

One question I have that I also asked in Slack is about moving images. We want to avoid re-compression of images because that leads to a loss of quality. The local dev environment tracks images it has compressed in _compress_images_cache.yml. Do you manually update this file when you move images to avoid re-compression?

guide.md Outdated Show resolved Hide resolved
guide/case-studies/savings-wallet.md Outdated Show resolved Hide resolved
guide/case-studies/upgradeable-wallet.md Outdated Show resolved Hide resolved
guide/daily-spending-wallet/first-use.md Outdated Show resolved Hide resolved
guide/daily-spending-wallet/landing-page.md Outdated Show resolved Hide resolved
guide/daily-spending-wallet/landing-page.md Outdated Show resolved Hide resolved
guide/daily-spending-wallet/landing-page.md Show resolved Hide resolved
guide/how-it-works/introduction.md Outdated Show resolved Hide resolved
guide/onboarding/introduction.md Show resolved Hide resolved
@GBKS GBKS added Copy Task is about improving text. Design Task is about designing something. labels Apr 5, 2022
@Bosch-0 Bosch-0 requested a review from GBKS April 5, 2022 10:27
@GBKS
Copy link
Contributor

GBKS commented Apr 5, 2022

ACK. Thanks for addressing all the changes so quickly.

Copy link
Contributor

@pavlenex pavlenex left a comment

Choose a reason for hiding this comment

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

LGTM

@pavlenex pavlenex dismissed sbddesign’s stale review April 5, 2022 11:44

Outdated, now :)

@pavlenex pavlenex merged commit 7f66de9 into BitcoinDesign:master Apr 5, 2022
@Mylenek
Copy link

Mylenek commented Apr 5, 2022

Info GFX

Hello @Bosch-0 ! As openly requested, I took a look at the DSW section and have to tell you all I am very impressed that so much information was distilled and captured so very well! I especially like the info graphic [captured here]. What a great device to visually guide the viewer through the content.

I will post 4 comments following this one with visuals that communicate some of my observations. The nature of my comments are based on a few minor improvements that I feel may want to be considered by the team. I hope my eye isn't too critical, if so, just reply and I'll adjust the magnification ; )

Great job to everyone involved...cheers!

@Mylenek
Copy link

Mylenek commented Apr 5, 2022

First use

@Mylenek
Copy link

Mylenek commented Apr 5, 2022

Back up and recovery

This particular image communicates the idea well but visually feels imposing due to it's scale and bold color.

@Mylenek
Copy link

Mylenek commented Apr 5, 2022

Requesting bitcoin

A perfectly fine graphic that [with a bit of cropping] could become more harmonious with the other visuals in the deck.

@Mylenek
Copy link

Mylenek commented Apr 5, 2022

Floating text

Here, I pulled 3 examples of callouts that have "floating" text... there are other instances in this section where this occurs but I'm certain you get the point. Consider adjusting indent or character spacing in order to modify the text, thereby eliminating the extra line [one "floating" word] in the callout.

@Mylenek
Copy link

Mylenek commented Apr 5, 2022

FYI - I'm new to GitHub so if there is a better way to communicate or use this platform please let me know and I'll adjust.
THX!

@GBKS
Copy link
Contributor

GBKS commented Apr 6, 2022

@Mylenek thanks for the feedback. For the point about floating words (aka orphans), that's something we discussed before. It's just one of those things about responsive design where you don't have full control over every detail. Text wraps differently based on screen size and user font settings via the browser or operating system. There's some info here in the Apple Human Interface Guidelines.

So manually tweaking text would only fix this for a specific configuration. Two other ways to fix it are via Javascript, like the NY times text balancer, or manually inserting non-breaking spaces between the last two words of every paragraph which is a bit messy. We briefly discussed these, but it's just not been a priority so far. Maybe we need to pick this up again sometime.

@GBKS
Copy link
Contributor

GBKS commented Apr 6, 2022

Also, good points about the images. Nice thing about open-source is that we can continuously tweak things. I'll create issues for each of these and then we can address them.

@Mylenek
Copy link

Mylenek commented Apr 6, 2022

Thanks for taking the time to thoughtfully reply @GBKS I'm learning & appreciate your consideration 👊

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Copy Task is about improving text. Design Task is about designing something.
Projects
None yet
5 participants