-
Notifications
You must be signed in to change notification settings - Fork 369
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
Clean up dialogs #6661
Clean up dialogs #6661
Conversation
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.
Reviewed 17 of 25 files at r1, all commit messages.
Reviewable status: 17 of 25 files reviewed, 1 unresolved discussion (waiting on @Pururun)
-- commits
line 5 at r1:
typo: dialogs
Code quote:
dialog
3497938
to
f20b403
Compare
f20b403
to
0efa5e6
Compare
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.
Reviewed 3 of 25 files at r1, 22 of 22 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @Pururun)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/NegativeConfirmationDialog.kt
line 50 at r4 (raw file):
fun NegativeConfirmationDialog( message: String, messageStyle: TextStyle? = null,
Shouldn't these be the same for all NegativeConfrimationDialog? Or when do we change the message style? I saw the NoEmail for error report seems to have a different style.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/NegativeConfirmationDialog.kt
line 51 at r4 (raw file):
message: String, messageStyle: TextStyle? = null, errorMessage: String?,
Maybe null as default? Then we don't have to define it on the call side if it does not have a message.
5f5ec68
to
4750d20
Compare
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.
Reviewable status: 16 of 25 files reviewed, 3 unresolved discussions (waiting on @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/NegativeConfirmationDialog.kt
line 50 at r4 (raw file):
Previously, Rawa (David Göransson) wrote…
Shouldn't these be the same for all NegativeConfrimationDialog? Or when do we change the message style? I saw the NoEmail for error report seems to have a different style.
It is used by NoEmail
and RemoveDevice
. In the case of no email it is because the text is quite long, in the case of remove device it is because it has some text that is bold so the normal text needs to be non-bold.
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/NegativeConfirmationDialog.kt
line 51 at r4 (raw file):
Previously, Rawa (David Göransson) wrote…
Maybe null as default? Then we don't have to define it on the call side if it does not have a message.
Done.
2e15105
to
36a5b29
Compare
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.
Reviewable status: 16 of 26 files reviewed, 3 unresolved discussions (waiting on @Rawa)
android/app/src/main/kotlin/net/mullvad/mullvadvpn/compose/dialog/NegativeConfirmationDialog.kt
line 50 at r4 (raw file):
Previously, Pururun (Jonatan Rhodin) wrote…
It is used by
NoEmail
andRemoveDevice
. In the case of no email it is because the text is quite long, in the case of remove device it is because it has some text that is bold so the normal text needs to be non-bold.
I tested a few different configuration for this and I think we should keep it as is and then look into it again when we look into fonts and text styles.
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.
Reviewed 8 of 9 files at r5, 1 of 1 files at r6, 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @Pururun)
36a5b29
to
f06532c
Compare
818530e
to
dbc0f2d
Compare
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.
Reviewed all commit messages.
Reviewable status: complete! all files reviewed, all discussions resolved
dbc0f2d
to
d7394d0
Compare
Clean up task:
DeleteConfirmationDialog
DeleteConfirmationDialog
toNegativeConfirmationDialog
AnnotatedString
toNegativeConfirmationDialog
HtmlText
as the last instance where it was used was replaced withAnnotatedString
This change is