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

New teacher account deletion flow #23666

Merged
merged 9 commits into from
Jul 13, 2018
Merged

Conversation

maddiedierker
Copy link
Contributor

This is follow-up for #23651 and updates the account deletion confirmation modal for teachers (spec).

Before

screen shot 2018-07-12 at 1 38 20 pm

After

screen shot 2018-07-12 at 1 37 55 pm

Remaining Work

  • Add initial "allow students to create their own logins" informational modal for teachers
  • Add modals to Storybook

"deleteAccount_teacherWarning3": "that don’t have a personal login or aren’t in another teacher’s section. Please make sure you have the authority to delete these students’ education records before deleting your own account. ",
"deleteAccount_teacherWarning4": "Give these students a chance to keep using their Code.org accounts by ",
"deleteAccount_teacherWarning5": "sending home instructions for creating personal logins. ",
"deleteAccount_teacherWarning6": "Give them at least a few days to follow these instructions BEFORE you delete your account.",
Copy link
Contributor

Choose a reason for hiding this comment

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

@Hamms Sorry to bug you, but what would it take for us to get redacted markdown working for apps translations like this? We've been seeing this more and more in accounts/teacher dashboard tools, and are really looking forward to markdown support.

Copy link
Contributor

Choose a reason for hiding this comment

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

No worries! To clarify, do we actually need redacted markdown support or just markdown support?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think both in this case. most of the text just requires styling, but there are a few that are links that would need to be redacted

Copy link
Contributor

Choose a reason for hiding this comment

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

Translated markdown, which I assumed (for security reasons) meant we needed redacted markdown.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, looking closer at what you're doing here, it seems like the answer is "both".

I would, however, say they are two separate issues. Implementing redacted markdown will make sending stuff that contains things like URLs or other immutable/sensitive information off to translators safe and easy, but we will still need a way to render that markdown in an apps context

Copy link
Contributor

@islemaster islemaster left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks for pulling this together so quick!

allCheckboxesChecked = () => {
const {checkboxes} = this.state;
const uncheckedBoxes = _.filter(Object.keys(checkboxes), id => !checkboxes[id].checked);
return uncheckedBoxes.length === 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

const {checkboxes} = this.state;
return Object.keys(checkboxes).every(id => checkboxes[id].checked);

{i18n.deleteAccount_teacherWarning6()}
</p>
</div>
}
</div>
Copy link
Contributor

Choose a reason for hiding this comment

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

This might be a good candidate for extraction to helper components in this file; It's be nice to write

{this.props.isTeacher ? <TeacherWarning/> : <StudentWarning/>}

and then hide the i18n/formatting mess away.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good idea! i'll make that refactor

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.

None yet

3 participants