Skip to content
This repository was archived by the owner on Sep 11, 2024. It is now read-only.

Use BaseDialog for DeactivateAccountDialog#1631

Closed
aidalgol wants to merge 1 commit into
matrix-org:developfrom
aidalgol:deactivate-account-dialog-fix
Closed

Use BaseDialog for DeactivateAccountDialog#1631
aidalgol wants to merge 1 commit into
matrix-org:developfrom
aidalgol:deactivate-account-dialog-fix

Conversation

@aidalgol
Copy link
Copy Markdown

Alter DeactivateAccountDialog to derive from BaseDialog.

Fixes element-hq/element-web#5694.

Signed-off-by: Aidan Gauland aidalgol@fastmail.net

Alter DeactivateAccountDialog to derive from BaseDialog.
@pafcu
Copy link
Copy Markdown
Contributor

pafcu commented Nov 27, 2017

Isn't DeactivateAccountDialog basically a QuestionDialog with the addition of a password check? Would it be possible to either base this off QuestionDialog, or turn this into some sort of more generic QuestionWithPasswordConfirmationDialog (at some point in the future if not now?)

Copy link
Copy Markdown
Member

@dbkr dbkr left a comment

Choose a reason for hiding this comment

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

Looks like this will lose the red title from the dialog, This isn't necessarily a problem if everyone if OK with that?

@pafcu
Copy link
Copy Markdown
Contributor

pafcu commented Nov 27, 2017

Getting out of scope for this PR, but might make sense to have some sort of generic importance flag (which would control if you get a "normal" dialog, or red titles, big exclamation marks etc.)

@lukebarnard1
Copy link
Copy Markdown
Contributor

IMO, it'd be a simple enough change to have an optional property as @pafcu suggests that applies the danger CSS class in BaseDialog. However, this will conflict with #1652, so let's get that merged first, I think.

@dbkr dbkr added the blocked label Jan 5, 2018
@dbkr
Copy link
Copy Markdown
Member

dbkr commented Jan 5, 2018

Regrettably blocked behind #1652

@dbkr
Copy link
Copy Markdown
Member

dbkr commented Jan 11, 2018

Closing since this is now superseded by your other PR (#1674)

@dbkr dbkr closed this Jan 11, 2018
@aidalgol
Copy link
Copy Markdown
Author

Oops, sorry for the doubleup!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants