- Notifications
You must be signed in to change notification settings - Fork928
feat: have user type name of thing to delete for extra safety#4080
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
this PR title made me smile, idk why :D |
| }) | ||
| it("disables confirm button when the text field is filled incorrectly", async () => { | ||
| const { t } = i18next |
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.
❤️
| <Maybe condition={info !== undefined}> | ||
| <Typography className={styles.warning}>{info}</Typography> | ||
| </Maybe> | ||
| <Typography>{t("deleteDialog.confirm", { entity })}</Typography> |
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.
I love how we're not passing whole strings in so that the language will stay consistent.
| title, | ||
| description, | ||
| entity, | ||
| info, |
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.
What kind of info is this? A description?
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.
Yeah, it's intended to help the user identify the entity to delete, so they don't delete the wrong one by accident.@bpmct suggested this on the basis of an article about making the wrong repo private and thus losing all their github stars.
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.
It's the orange text here. Note storybook isn't showing buttons; that's been going on for a while and so far doesn't mean the buttons are actually gone.
| fireEvent.click(moreButton) | ||
| const moreButtons = await screen.findAllByLabelText("more") | ||
| const suspendedMoreButton = moreButtons[2] | ||
| fireEvent.click(suspendedMoreButton) |
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.
Curious why we're using afireEvent instead of anawait user.click like above.
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.
It should be fine to do so - I just didn't make as many changes in this test. I wanted to switch to getting the "more" buttons everywhere because they load later than users, but theuserEvent change was more of a hygiene thing than a cause of bugs.
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.
Got it! Thanks
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.
This was satisfying to review!!! Thanks for doing it.
Closes#3384