-
Notifications
You must be signed in to change notification settings - Fork 489
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: delete-modal pin guidance, improved language #1644
Conversation
@lidel -- this one is ready for you to wire up at your leisure 😊 |
Second option is blocked by #1615, I'd park this until we have pinning services wired up, then rebase this PR. |
@rafaelramalho19 I won't have bandwidth for pushing this over finish line any time soon, mind taking it over? |
@jessicaschilling maybe the first checkbox should be selected by default? |
@rafaelramalho19 Sounds good, please do! |
Quick note: per discussion in #1258 (comment), please include the name of the (file, folder) to be deleted if only one item is being deleted. This would mean first line of text would be: Are you sure you want to remove "catphoto.gif"? @rafaelramalho19 happy to make up another screenshot if you want it, but this is pretty self-explanatory 😊 |
@rafaelramalho19 - I rebased this against current master, so this should be relatively easy to finish up (please see the first comment for checklist of remaining items to do). It's one of the remaining issues in the pinning service integration epic, so please prioritize with that work. Thank you! |
@rafaelramalho19 mind rebasing this? I believe we want to land this along with #1721 |
@rafaelramalho19 #1713 just got merged, this PR should be unblocked now for you to rebase and resume work. I believe the behavior we want is:
|
bb5f79d
to
71a415a
Compare
@lidel ready for review 🎉 |
This ensures we don't remove pins if removal failed first, and also do pin removal in best-effort fashion to avoid crashing webui on DAGs with multiple instances of the same CID Also, closes #1740
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.
@rafaelramalho19 Thank you, works as expected (found edge case but fixed it in 5285a8b) 👌
The only remaining ask is to replace checkboxes with styled ones that we already use on Files list:
This makes filename more prominent when a single item is removed. Easier to scan when performing quick operations.
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.
Thanks! Works as expected.
Made two cosmetic tweaks:
(1) moved item name to modal title, so it is harder to miss:
(2) added truncation rule to the title so long filenames don't break the modal:
@jessicaschilling mind taking a quick look if above changes are ok?
@lidel Great idea 😊 Do you mind adding the question mark on the other side of the truncation |
Unfortunately i18next does not support ICU translations with both pluralizations AND embedded tags, and for truncating only the filename we would have to wrap it in a To avoid this mess, I can move the question mark before filename, so we don't need any additional tags. It would look like this: @jessicaschilling would that be better or worse? |
It's not visually pretty, but it's better semantically. Go for it. 😊 |
Summary
This PR prepares for pinning service continuity and improves overall UX of the Files screen "Delete" modal as follows:
Closes #1634.
Closes #1258
Done
actions.delete
withactions.remove
inapp.json
; make sure this doesn’t cause any other issuesdeleteModal
/DeleteModal
toremoveModal
/RemoveModal
and update file names accordinglyTo do
Open question
Should we change CLI tutor mode guidance in any way to match?
Screenshot
(Note this doesn't include the language Are you sure you want to remove "catphoto.gif"? - can update screenshot if desired)