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

Enhance file remove modal #895

Closed
tzarebczan opened this issue Dec 22, 2017 · 13 comments
Closed

Enhance file remove modal #895

tzarebczan opened this issue Dec 22, 2017 · 13 comments
Assignees
Labels
hacktoberfest Welcome to Hacktoberfest help wanted level: 0 needs: exploration Solution unclear, needs research Tom's Wishlist type: improvement Existing (or partially existing) functionality needs to be changed

Comments

@tzarebczan
Copy link
Contributor

tzarebczan commented Dec 22, 2017

The Issue

The current file remove modal can be confusing to users:

file remove

  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
Copy link
Contributor

Can I grab this one?

@kauffj
Copy link
Member

kauffj commented Oct 14, 2019

That'd be awesome @brentmclark !

@brentmclark
Copy link
Contributor

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
Copy link

neb-b commented Oct 15, 2019

@brentmclark Those are looking great!

@brentmclark
Copy link
Contributor

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
Copy link

neb-b commented Oct 15, 2019

@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
Copy link
Contributor

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

@brentmclark
Copy link
Contributor

@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
Copy link
Contributor

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

without error

with error

@neb-b
Copy link

neb-b commented Oct 16, 2019

That's looking great!

@brentmclark
Copy link
Contributor

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

@kauffj
Copy link
Member

kauffj commented Oct 16, 2019

A copy few suggestions:

  • URI and claim are both words we generally avoid forcing users to learn. Maybe "Abandon on blockchain"? It would also be neat if the amount returned could be shown, e.g. "Abandon on blockchain (receive 0.5 LBC back).
  • Remove the word "also" for the first checkbox

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
Copy link

e4drcf commented May 18, 2020

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

@neb-b neb-b closed this as completed May 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest Welcome to Hacktoberfest help wanted level: 0 needs: exploration Solution unclear, needs research Tom's Wishlist type: improvement Existing (or partially existing) functionality needs to be changed
Projects
None yet
Development

No branches or pull requests

6 participants