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

feat: add cheatsheet #211

Merged
merged 28 commits into from
Apr 14, 2022
Merged

feat: add cheatsheet #211

merged 28 commits into from
Apr 14, 2022

Conversation

theborakompanioni
Copy link
Collaborator

@theborakompanioni theborakompanioni commented Apr 5, 2022

This PR adds a cheatsheet component.

It is currently only shown in devMode for now, as some items on the list (e.g. Fidelity Bonds), are not yet supported.

Differences to the Figma design:

  • Adds a close button to the overlay - this is needed on small mobile devices (Should it be hidden on bigger screens?)
  • Footer Link Icon does not match (currently using File as placeholder – found no real alternative on https://bitcoinicons.com/ yet)

📸

@theborakompanioni theborakompanioni added enhancement New feature or request concept Wild idea, or too many details unknown yet WIP Work in Progress labels Apr 5, 2022
@theborakompanioni theborakompanioni self-assigned this Apr 5, 2022
@theborakompanioni theborakompanioni marked this pull request as ready for review April 8, 2022 09:27
@theborakompanioni theborakompanioni requested a review from a user April 8, 2022 09:27
@theborakompanioni
Copy link
Collaborator Author

Footer Nav.Item's still need some styling on mobile.

@editwentyone
Copy link

editwentyone commented Apr 8, 2022

thanks for the update. i added a "close" button and changed icon to the "file" icon from bitcoinicons. valid point for mobile use.

some thoughts:

would love to see the cheatsheet jump up once fully automatically after a delay when the user uses JAM for the first time, directly after the creation of the first wallet (see rapid prototype, flow "empty wallet" for a better understanding, press R to reset prototype).

the underlined words are links, the first 3 links in the text are linking to openoms sources and the other links are linking to parts inside the jam app. i.e.: (1) Fund your wallet first. (click it inside the prototype)

also its better to hide "cheatsheet" in the footer until the user created the first wallet (if easy and possible). just to not confuse too much at the beginning.

I gave point 4 fidelity bond another color to show its inactive for now, but never the less, there are some informations that enlightens the user. maybe its a good idea to also communicate the upcoming feature FB

src/components/Cheatsheet.tsx Outdated Show resolved Hide resolved
src/index.css Show resolved Hide resolved
return <div className="numbered">{number}</div>
}

function ListItem({ number, children }: PropsWithChildren<NumberedProps>) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you can take this one step further

interface ListItemProps {
  number: number
  transaltionKey: string
  descriptionKey: string
}

function ListItem({ number, children, transaltionKey, descriptionKey }: PropsWithChildren<ListItemProps>) {
  const { t } = useTranslation()
  return (
    <rb.Stack className="cheatsheet-list-item" direction="horizontal" gap={3}>
      <div className="numbered">{number}</div>
      <rb.Stack gap={0}>
        <h6>
          <Trans i18nKey={transaltionKey}>{children}</Trans>
        </h6>
        <div className="small text-secondary">{t(descriptionKey)}</div>
      </rb.Stack>
    </rb.Stack>
  )
}
<ListItem number={1} transaltionKey="cheatsheet.item_1.title" descriptionKey="cheatsheet.item_1.description">
    <Link to="/receive">Fund</Link> your wallet first.
</ListItem>

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Passing translation keys strikes me as unnecessarily restrictive.
It is so little code - feels like over-engineering to me for now. Do not get me wrong - I love to over-engineer.. : D
But feel free to add any improvements once the PR is merged!

Co-authored-by: Đorđe Spasić <34274884+Djo1e@users.noreply.github.com>
@theborakompanioni
Copy link
Collaborator Author

Updated screenshots.

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Apr 11, 2022

would love to see the cheatsheet jump up once fully automatically after a delay when the user uses JAM for the first time, [...]

Done. Will be shown once after a wallet has been unlocked, just like the onboarding screen.

the underlined words are links, the first 3 links in the text are linking to openoms sources and the other links are linking to parts inside the jam app. i.e.: (1) Fund your wallet first. (click it inside the prototype)

👍

also its better to hide "cheatsheet" in the footer until the user created the first wallet (if easy and possible). just to not confuse too much at the beginning.

It will be displayed only if a wallet is unlocked - not on the login screen (links would not work).

I gave point 4 fidelity bond another color to show its inactive for now, but never the less, there are some informations that enlightens the user. maybe its a good idea to also communicate the upcoming feature FB

👍 Done. Opacity decreased for item 4.

Edit: The feature is still disabled by default for now - should it be enabled once scheduled transactions are supported?

@theborakompanioni theborakompanioni removed the WIP Work in Progress label Apr 11, 2022
@theborakompanioni
Copy link
Collaborator Author

Nice! That looks great already!

All honor to @editwentyone.

Some word choices could be improved, or at least we should explain the concepts somewhere.

I am always grateful for concrete wording suggestions, because there are so many people who have the necessary talent for this - but I am afraid I am not one of them.

Here is what came to mind:

* Why is "adding noise" good?

Changed from "add more noise" to "increase privacy of all". "noise" is avoided entirely. It is not used anywhere and would - like you said - need an explanation. Acceptable approach?

* What is "sweep"?

Since we use the term "sweep" on the Send screen also, do you think it can be explained there and is this the appropriate context? Refrained from changing anything in the cheatsheet component regarding this for now.

* "very private" - how about "more private"?

👍 "more private" it is.

* Not sure if we should focus on yield that heavily, as others mentioned in the past the amounts that you will earn are probably rather low. Maybe do some expectation management?

Yep, that is true. Maybe also better explained on the "Earn" screen, when the context is appropriate (?).

Feel free to ignore the above, we can improve all this at a later point as well. It's a good first version! clap

One question before it is merged - should item 3 and 4 be flipped?

@dergigi
Copy link
Contributor

dergigi commented Apr 12, 2022

Should item 3 and 4 be flipped?

I'd say yes.

Another thing that touches on something that we discussed often—something that isn't fully resolved yet—is the mentioning of "levels."

If we manage to implement and ship #179 with the next release, manual consolidation (and thus anything that mentions "levels" at all) might fall to the wayside for most users, since this would be automated with scheduled transactions.

I'll propose some wording changes in the code directly.

theborakompanioni and others added 2 commits April 12, 2022 14:39
Co-authored-by: Gigi <109058+dergigi@users.noreply.github.com>
Co-authored-by: Gigi <109058+dergigi@users.noreply.github.com>
@dergigi
Copy link
Contributor

dergigi commented Apr 12, 2022

I just noticed that there are two sources of truth for the text due to the translation engine, so I will just type it out here before I propose code changes that might be misplaced:


The Cheatsheet

Follow the steps below to increase your financial privacy. It is advisable to switch from earning as a maker to sending as a taker back and forth. Learn more.

1) Fund your wallet.
Deposit some funds yourself or receive it from others.

2) Send a collaborative transaction to yourself.
Collaborative transactions increase everyone's privacy.

3) Optional: Lock funds in a fidelity bond.
A fidelity bond increases your chances of selling liquidity substantially.

4) Provide liquidity and earn yield.
Offer your sats to the marketplace. No trust or custody required—you are always in full control of your funds.

5) Make use of scheduled transactions
Automatically plan and execute a series of collaborative transactions. You can also automatically sweep your funds to cold storage, or use them to open a lightning channel, for example.

6) Go to step one and repeat.
Still confused? Dig into the documentation.


This assumes that we will ship #179 soon, which I think we will. I'm an optimist! 😁

Let me know what you think.

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Apr 12, 2022

I just noticed that there are two sources of truth for the text due to the translation engine, so I will just type it out here before I propose code changes that might be misplaced.

Ah, yes, totally forgot that fact when applying the suggestions before.

This assumes that we will ship #179 soon, which I think we will. I'm an optimist! grin

Yes. Worst case is, we have to apply css class upcoming-feature to that item. Not an issue 👍

Let me know what you think.

I'd merge it! Ping @editwentyone for a competent design perspective.

@editwentyone
Copy link

hey :) because on mobile its a lot of text, I would suggest to fix the Numbers at the top of the cell, I also fixed it in Figma.

also I would suggest to link only the important words like Fund, Send, Earn, it reads faster

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Apr 13, 2022

hey :) because on mobile its a lot of text, I would suggest to fix the Numbers at the top of the cell, I also fixed it in Figma.

👍 Done.

also I would suggest to link only the important words like Fund, Send, Earn, it reads faster

👍 Done.
I changed the wording slightly in order for links to come first (at least in English).

  • "Provide liquidity and earn yield." => "Earn yield by providing liquidity."
  • "Make use of scheduled transactions." => "Use scheduled transactions."

@dergigi
Copy link
Contributor

dergigi commented Apr 13, 2022

Love it! One final suggestion from my side:

  1. Schedule transactions.

This way it reads fund / send / earn / schedule.

Great suggestions Edi, and great job implementing all this tbk! 🧡

@theborakompanioni
Copy link
Collaborator Author

theborakompanioni commented Apr 13, 2022

This way it reads fund / send / earn / schedule.

Looks and reads even better!

The latest changes also contain the most basic support for simple Feature Flags.
Feature 'cheatsheet' can be enabled once #179 is merged.

Will merge this as soon as it is approved, but please do not hold back in case anyone still comes up with suggestions or feedback.

Copy link
Contributor

@dergigi dergigi left a comment

Choose a reason for hiding this comment

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

Looks good to me ✅

@theborakompanioni theborakompanioni merged commit 825f725 into master Apr 14, 2022
@theborakompanioni theborakompanioni deleted the cheatsheet branch April 14, 2022 09:50
@editwentyone
Copy link

one little annoyance: by clicking a link like "Fund", its switching the page in the background and keeps the cheatsheet still in the front. could we close the cheatsheet also with the same click?

(+ is it possible to overlay the complete page including the header when the cheatsheet is active?)

@theborakompanioni
Copy link
Collaborator Author

one little annoyance: by clicking a link like "Fund", its switching the page in the background and keeps the cheatsheet still in the front. could we close the cheatsheet also with the same click?

I thought of that as a feature instead of a bug : D
But of course, it is possible! Would you open a ticket for that? Could be tagged with "good first issue".

(+ is it possible to overlay the complete page including the header when the cheatsheet is active?)

This is a little bit trickier, but also possible of course. The reason is that the navbar should be shown as usual on some occasions when a backdrop is active, e.g. when showing the navigation on mobile. Personally, if I may say that, I'd rather not do that.

@theborakompanioni
Copy link
Collaborator Author

one little annoyance: by clicking a link like "Fund", its switching the page in the background and keeps the cheatsheet still in the front. could we close the cheatsheet also with the same click?

I thought of that as a feature instead of a bug : D But of course, it is possible! Would you open a ticket for that? Could be tagged with "good first issue".

This should now also work with v0.0.6 @editwentyone 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concept Wild idea, or too many details unknown yet enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants