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

Minimal warning if student has never signed in #23625

Merged
merged 6 commits into from Jul 11, 2018

Conversation

islemaster
Copy link
Contributor

@islemaster islemaster commented Jul 11, 2018

Show a much less scary confirmation dialog when removing a student who has never signed in. (spec). This is a follow-up to #23619.

Also added a story for this component since it now has multiple states:
screenshot from 2018-07-11 11-46-09

@islemaster islemaster changed the title [WIP] Minimal warning if student has never signed in Minimal warning if student has never signed in Jul 11, 2018
@islemaster islemaster changed the base branch from remove-student-warning to staging July 11, 2018 18:52
@islemaster islemaster mentioned this pull request Jul 11, 2018
Copy link
Contributor

@maddiedierker maddiedierker left a comment

Choose a reason for hiding this comment

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

👍 i think we'll want to get some tests on ConfirmRemoveStudentDialog now that we're introducing additional states (rather than a basically static modal), but not a blocker for merging since this is behind a flag

@islemaster
Copy link
Contributor Author

Hah! I actually wrote unit tests for it, then removed them in the last commit here because, from a coverage perspective, they hit exactly the same code the story does. They do make stronger assertions about the contents of the dialog though; I'm happy to put them back if you think they are useful.

@maddiedierker
Copy link
Contributor

haha, i hadn't noticed that! storybook will catch if the component errors on render, but the tests will catch that the contents are correct, so i'd say add them back. if it were just the two states, i'd say leave the tests off, but since we're adding a third state if the student has their own login, having coverage on the modal content becomes more useful.

@islemaster islemaster force-pushed the remove-student-warning-types branch from e816bcb to caedac2 Compare July 11, 2018 20:00
@islemaster islemaster merged commit 2dcf80e into staging Jul 11, 2018
@islemaster islemaster deleted the remove-student-warning-types branch July 11, 2018 21:24
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

2 participants