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

Update language about delete tasks #8065

Conversation

marcellamaki
Copy link
Member

@marcellamaki marcellamaki commented May 7, 2021

Summary

This PR updates the modals for deletion tasks to include a warning that tasks cannot be cancelled or undone, as applicable.
Fixes #6727

Example of before/after from one of the modals

Before:
Screen Shot 2021-05-07 at 12 44 43 PM

After:
Screen Shot 2021-05-07 at 12 43 07 PM

Reviewer guidance

You can confirm the new strings do not have any typos :) and manually QA by checking the modals that pop up when deleting:

  • Channel(s)
  • Resource(s)
  • Group
  • Lesson
  • Exam
  • Class
  • User

Testing checklist

  • Contributor has fully tested the PR manually
  • If there are any front-end changes, before/after screenshots are included
  • Critical user journeys are covered by Gherkin stories
  • Critical and brittle code paths are covered by unit tests

PR process

  • PR has the correct target branch and milestone
  • PR has 'needs review' or 'work-in-progress' label
  • If PR is ready for review, a reviewer has been added. (Don't use 'Assignees')
  • If this is an important user-facing change, PR or related issue has a 'changelog' label
  • If this includes an internal dependency change, a link to the diff is provided

Reviewer checklist

  • Automated test coverage is satisfactory
  • PR is fully functional
  • PR has been tested for accessibility regressions
  • External dependency files were updated if necessary (yarn and pip)
  • Documentation is updated
  • Contributor is in AUTHORS.md

@radinamatic
Copy link
Member

radinamatic commented May 11, 2021

Thank you, Marcella! 👍🏽
I know I was the one who initially reported the inability to cancel as problematic, but I'm somewhat unsure about the necessity to alert about it, if the warning about inability to undo is already in place.

In the first place, to increase the visibility of the warning, the phrase This action cannot be undone/cancelled should be at the bottom of the modal, as close as possible to the (actionable) button Delete. This is especially valid for modals with several phrases, as it probably happened to all of us to skip reading all in a hurry, disregard the warnings, and 😱 afterwards.

Selection_024

If that last warning can be in bold font, even better.

On the other hand, having the warning This action cannot be cancelled so close to the Cancel button seems counterintuitive and could be confusing. For that reason, since the end result is identical for the user (who cannot neither cancel nor revert), I think it would be best just to keep the same This action cannot be undone warning in all the confirmation modals for deleting tasks.

This action cannot be undone alert should be in the new line at the end.

Lesson ❌ Quiz ❌ Group ✔️
Selection_027 Selection_028 Selection_029

Anti-MG-rant not specific to this PR (I should open the discussion in the KDS).

Is it just my eyes that bleed when I see full phrases without the proper full stop at the end? 😕

Looking at a question phrase ending with a proper ?, followed by a full phrase ending without a proper ., followed by another phrase with a proper . as above, just makes me cringe.

I know that is what the MG recommends, but since not even Google follows them consistently in their flagship product, seems like we are trying to be bigger Catholics than the pope.

Gmail Settings Gmail Settings 2
Selection_025 Selection_026

@marcellamaki
Copy link
Member Author

Thanks so much for your detailed feedback @radinamatic! This is really helpful both for this PR and for me as I continue to learn about UI - so many ideas to keep for future reference 😊

I'll work on implementing these updates today. And, aside, I totally agree with finding the lack of a "." at the end of complete sentences to be extremely weird, even though I know it's in the material guidelines. Maybe something to raise at a KDS meeting? 😉

@jonboiser jonboiser added this to the 0.15.0 milestone May 11, 2021
@marcellamaki marcellamaki force-pushed the warn-users-deletion-tasks-no-cancellation branch from 09659c4 to 364b1e5 Compare May 12, 2021 23:02
Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

One thing to note is that since these strings are the exact same in all cases and used across the whole coach and device, we should add the string to the commonCoreStrings.js file and then use them in the components using the coreString method on that mixin like it already is being used in DeleteResourcesModal

import commonCoreStrings from 'kolibri.coreVue.mixins.commonCoreStrings';
export default {
name: 'DeleteResourcesModal',
mixins: [commonCoreStrings],

So if you add that string to the common module, it'll be available to any component mixing in that commonCoreStrings module.

I also suggest maybe changing the noUndo key to something a bit clearer about the purpose of the string. cannotUndoActionWarning or something similar might make it a bit clearer to the team what the purpose of that string is. It also helps avoid ambiguity when strings might have identical literal English text, but the translation might differ depending on context.

Additionally, when you create the string, you can add some context to it as well like the snippet below. I don't think it's necessary in this situation, especially if the key is renamed to something more descriptive, but just wanted to share:

registerAction: {
message: 'Register',
context: 'Register a facility to the Kolibri Data Portal',
},

@@ -66,6 +67,7 @@
assignmentQuestion: 'Assign quiz to',
deleteExamTitle: 'Delete quiz',
deleteExamDescription: "Are you sure you want to delete '{ title }'?",
noUndo: ' This action cannot be undone',
Copy link
Member

Choose a reason for hiding this comment

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

There is a space between the opening ' and This here.

@@ -42,6 +43,7 @@
$trs: {
modalTitle: 'Delete class',
confirmation: "Are you sure you want to delete '{ classname }'?",
noUndo: ' This action cannot be undone',
Copy link
Member

Choose a reason for hiding this comment

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

Space here as well

@radinamatic
Copy link
Member

radinamatic commented May 13, 2021

➕ ➕ ➕ to all 3 suggestions by @nucleogenesis :

  • include This action cannot be undone in commonCoreStrings
  • name the string so it reflects the purpose as clearly as possible
  • add the context with every new string

Copy link
Member

@nucleogenesis nucleogenesis left a comment

Choose a reason for hiding this comment

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

Changes look great! Thanks Marcella!

@rtibbles rtibbles merged commit 783a7ac into learningequality:develop May 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users should be warned that resource deletion tasks cannot be canceled
5 participants