lbryio / lbry-desktop

A browser and wallet for LBRY, the decentralized, user-controlled content marketplace.
https://lbry.tech
MIT License
3.56k stars 414 forks source link

Enhance file remove modal #895

Closed tzarebczan closed 4 years ago

tzarebczan commented 6 years ago
## The Issue The current file remove modal can be confusing to users: ![file remove](https://user-images.githubusercontent.com/8120721/34314317-5ffa04ae-e740-11e7-9ecd-55e4c5088a63.png) 1) "from LBRY?" should probably be closer to something like "from the app", especially on claims that you don't own. 2) show the file path/name somewhere 3) rewording of claim abandoning, check on by default. Warn if unchecking - the file is removed from their publishes area and the user can't remove the deposit from the claim page anymore (needs to be deleted via transaction list or redownloaded). ## System Configuration

Anything Else

Screenshots

brentmclark commented 5 years ago

Can I grab this one?

kauffj commented 5 years ago

That'd be awesome @brentmclark !

brentmclark commented 5 years ago

Thanks @kauffj! I could use a little help with the verbiage, though. I'm not particularly familiar with this product.

So far, I have this when checked:

state when checked

and this when unchecked:

state when unchecked

neb-b commented 5 years ago

@brentmclark Those are looking great!

brentmclark commented 5 years ago

Thanks, @seanyesmunt! I still have some work to do to get the strings into app-strings.json and the styles into the SCSS files. The app-strings implementation looks pretty straightforward, but I could use some help finding the optimal place for my styles.

For example, the red copy above. Can I add a --color-warning variable to the _vars.scss file or should this be treated as a one-off?

Similarly, for the form field descriptions that rest atop their respective form fields; I see you're using BEM. Does it make sense to extend the fieldset-section block with something like fieldset-section__description and fieldset-section__description--subtext?

neb-b commented 5 years ago

@brentmclark The app-strings will be populated automatically so you don't need to worry about those.

You can just use .error-text for the red text.

For the additional text we should probably just use the helper prop, since we use that in other places. Maybe both the error and description text should be passed as helper? Not sure how that would look.

brentmclark commented 5 years ago

@seanyesmunt Thanks for the quick feedback; I'll give these a shot tonight.

brentmclark commented 5 years ago

@seanyesmunt .error-text worked great; thanks!

The helper prop renders content below the checkbox instead of above the checkbox. To me, it doesn't read properly when rendered below the checkbox in this instance. Thoughts?

helper text rendered below checkbox

brentmclark commented 5 years ago

Oh my goodness I'm dense 😛. How do you feel about this approach instead?

without error

with error

neb-b commented 5 years ago

That's looking great!

brentmclark commented 5 years ago

Awesome. I'll cut the PR tonight. Thanks for your help!

kauffj commented 5 years ago

A copy few suggestions:

The vertical spacing is also a bit inconcistent, but that doesn't necessarily need to be fixed and can probably be left to @seanyesmunt

e4drcf commented 4 years ago

may be it's time to close this issue?) it's in the master already)