-
Notifications
You must be signed in to change notification settings - Fork 414
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
Onboarding + youtube transfer + channels page #2925
Conversation
b724046
to
106baa9
Compare
return ( | ||
<Form onSubmit={handleSubmit}> | ||
<Card | ||
title={__('Enter Your LBRY Password')} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably could use some help text here? This is when you are logging in to an account that was encrypted with a password. You need to use the password you used on the previous device
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Wallet Password [?]"
[?]: Your wallet password was set when you previously installed LBRY on another device.
where [?] = mouseoverable help thing
Does it have to be another device though? Is wallet password put into OS keyring? If not, couldn't it be the same machine/install?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, it could be saved from an old install on the same device.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just say the following, it's nice and ambiguous:
[?]: You set your wallet password when you previously installed LBRY.
name="sync_checkbox" | ||
label={__('Sync your balance between devices')} | ||
helper={ | ||
balance > 0 ? __('This is only available for empty wallets') : __('Maybe some more text about something') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it only available for empty wallets?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a current restriction of the sync API, though I don't think it will be like that for long.
af6f155
to
cb0387c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly requested copy, did not look at most of PR
nice to have:
bugs:
|
another small thing - a channel pending confirmation refreshes repeated on channels page. We may eventually have to do something similar to pending publishes. |
6db47e7
to
f63d728
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Generally looks good.
@@ -219,7 +232,7 @@ const ClaimPreview = forwardRef<any, {}>((props: Props, ref: any) => { | |||
</div> | |||
)} | |||
</div> | |||
<ClaimTags uri={uri} type={type} /> | |||
{properties !== undefined ? properties : <ClaimTags uri={uri} type={type} />} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should properties be when you want the positive action to happen? I suspect this check should be stronger
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mainly properties={false}
to hide them.
this.setState({ | ||
message: __('Blockchain Sync'), | ||
details: `${__('Catching up...')} (${__(format, wallet.blocks_behind)})`, | ||
details: `${__('Catching up...')} (${__(amountBehind, { amountBehind: wallet.blocks_behind })})`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should just be __(
now, not ${__(
, right?
(Also, this is where __n
could be useful if we want it)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We still need the $
because it's wrapped in a template string
src/ui/component/userSignIn/view.jsx
Outdated
const hasYoutubeChannels = youtubeChannels && Boolean(youtubeChannels.length); | ||
const hasTransferrableYoutubeChannels = hasYoutubeChannels && youtubeChannels.some(channel => channel.transferable); | ||
const hasPendingYoutubeTransfer = | ||
hasYoutubeChannels && youtubeChannels.some(channel => channel.transfer_state === 'pending_transfer'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'pending_transfer' should probably be a lbryio constant :/
) : ( | ||
<section className="main--empty"> | ||
<div className=" section--small"> | ||
<h2 className="section__title--large">{__('No Channels Created Yet')}</h2> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
stylistically I think empty states could still use work
because the state is empty, anything being displayed already has a lot of emphasis
this empty state (and some others) use large bold text, I think this could be softened/reduced
I also think more horizontal space could be used before wrapping
Maybe look at https://material.io/design/communication/empty-states.html for how Material Design does it
6d62207
to
064b47e
Compare
f690545
to
c1f25e5
Compare
Cleanup from the initial MVP
Before you review
<Card />
component. You can skippage/settings/view.jsx
andpage/help/view.jsx
since I just moved stuff around.Changes
util/
and intoeffects/
Card
component.cardxx
styles.card--xxx
references on non-card elements to use the more generic.section--xx
classes