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

[dashboard] Harmonize confirmation dialog #4408

Merged
merged 2 commits into from
Jun 8, 2021

Conversation

corneliusludmann
Copy link
Contributor

@corneliusludmann corneliusludmann commented Jun 7, 2021

There are confirmation dialogs on different places where the code has be copied. In some cases, small deviations have been introduced unintentionally (slightly different shade of gray, etc.). For this reason, this PR introduces a common ConfirmationModal.

This fixes also #4264, and #3701.

/cc @gtsiolis

Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

LGTM.
Some minor comments

components/dashboard/src/settings/Account.tsx Outdated Show resolved Hide resolved
components/dashboard/src/components/ConfirmationModal.tsx Outdated Show resolved Hide resolved
@corneliusludmann corneliusludmann force-pushed the corneliusludmann/dashboard-dark-4264 branch from 5b6ee0a to a7a1674 Compare June 7, 2021 15:10
@corneliusludmann corneliusludmann force-pushed the corneliusludmann/dashboard-dark-4264 branch from a7a1674 to 463148a Compare June 7, 2021 15:40
@corneliusludmann corneliusludmann merged commit fdb332b into main Jun 8, 2021
@corneliusludmann corneliusludmann deleted the corneliusludmann/dashboard-dark-4264 branch June 8, 2021 11:32
@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 8, 2021

Thanks @corneliusludmann for fixing and @JanKoehnlein for reviewing this!
Both #4264 and #3701 seem to be resolved now. 🍖 🍗


export default function ConfirmationModal(props: {
title?: string;
areYouSureText?: string,
Copy link
Contributor

Choose a reason for hiding this comment

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

praise: Nice one! Thanks for adding this abstraction based on the existing modals. 🔝

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants