Skip to content

Conversation

amrbashir
Copy link
Member

closes #2640

@amrbashir amrbashir requested a review from a team as a code owner April 17, 2025 03:13
Copy link
Contributor

github-actions bot commented Apr 17, 2025

Package Changes Through 355fc91

There are 4 changes which include dialog with minor, dialog-js with minor, updater with minor, updater-js with minor

Planned Package Versions

The following package releases are the planned based on the context of changes in this pull request.

package current next
api-example 2.0.35 2.0.36
api-example-js 2.0.31 2.0.32
dialog 2.3.3 2.4.0
dialog-js 2.3.3 2.4.0
updater 2.9.0 2.10.0
updater-js 2.9.0 2.10.0

Add another change file through the GitHub UI by following this link.


Read about change files or the docs at github.com/jbolda/covector

@amrbashir
Copy link
Member Author

iOS is not tested ofc

Windows and Android are tested and works.

@amrbashir amrbashir requested a review from Legend-Master April 19, 2025 11:34
@amrbashir
Copy link
Member Author

@FabianLars or @lucasfernog can you test the iOS stuff?

@FabianLars
Copy link
Member

Can do but I'm kinda away over the weekend so please remind me on Monday if I or Lucas didn't get to it yet by then.

@amrbashir
Copy link
Member Author

don't worry about it, enjoy your weekend

@lucasfernog
Copy link
Member

I started taking a look at it but got carried away to other things. I was trying to improve the return type but.. it's complicated

Legend-Master
Legend-Master previously approved these changes Apr 19, 2025
Copy link
Contributor

@Legend-Master Legend-Master left a comment

Choose a reason for hiding this comment

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

Desktop code looks good, tested on Windows

Comment on lines 96 to 125
export type MessageDialogButtonsYesNoCancel = {
/** The Yes button. */
yes: string
/** The No button. */
no: string
/** The Cancel button. */
cancel: string
}

/**
* The Ok and Cancel buttons of a message dialog.
*
* @since 2.3.0
*/
export type MessageDialogButtonsOkCancel = {
/** The Ok button. */
ok: string
/** The Cancel button. */
cancel: string
}

/**
* The Ok button of a message dialog.
*
* @since 2.3.0
*/
export type MessageDialogButtonsOk = {
/** The Ok button. */
ok: string
}
Copy link
Contributor

@Legend-Master Legend-Master Apr 19, 2025

Choose a reason for hiding this comment

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

Just a small suggestion, maybe we can change these to something like this, so typescript doesn't hint ok and yes at the same type

Suggested change
export type MessageDialogButtonsYesNoCancel = {
/** The Yes button. */
yes: string
/** The No button. */
no: string
/** The Cancel button. */
cancel: string
}
/**
* The Ok and Cancel buttons of a message dialog.
*
* @since 2.3.0
*/
export type MessageDialogButtonsOkCancel = {
/** The Ok button. */
ok: string
/** The Cancel button. */
cancel: string
}
/**
* The Ok button of a message dialog.
*
* @since 2.3.0
*/
export type MessageDialogButtonsOk = {
/** The Ok button. */
ok: string
}
export type MessageDialogButtonsYesNoCancel = {
/** The Yes button. */
yes: string
/** The No button. */
no: string
/** The Cancel button. */
cancel: string
}
/**
* The Ok and Cancel buttons of a message dialog.
*
* @since 2.3.0
*/
export type MessageDialogButtonsOkCancel = {
/** The Ok button. */
yes: string
/** The Cancel button. */
cancel: string
}
/**
* The Ok button of a message dialog.
*
* @since 2.3.0
*/
export type MessageDialogButtonsOk = {
/** The Ok button. */
yes: string
}

@FabianLars
Copy link
Member

@lucasfernog

I was trying to improve the return type but.. it's complicated

Can you maybe expand on that so someone else can potentially take over?

@FabianLars
Copy link
Member

i love this :D
grafik

@lucasfernog
Copy link
Member

i love this :D grafik

😂 😂 omg it took me a minute to realize wtf is this

lucasfernog
lucasfernog previously approved these changes Aug 26, 2025
Copy link
Member

@lucasfernog lucasfernog 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 can go with this now

@Legend-Master
Copy link
Contributor

Legend-Master commented Aug 26, 2025

To be honest, although it's correct at the type level and it does error out if extra buttons are passed, this type makes it harder to follow to me

image

instead of

image

@lucasfernog
Copy link
Member

weird, i only ever got the second type of warning.. maybe we just go with the other way :|
i didn't really like using ok instead of yes because then it wouldn't match the rust side.. and ok / no / cancel is a weird trio IMO, but maybe that's just me

@Legend-Master
Copy link
Contributor

I feel like ok / no / cancel is ok because most of the time, 3 buttons will be 1 primary action 1 secondary action and cancel, it's not really yes / no / cancel

But this doesn't quite match the rfd's default YesNoCancel so...

I feel like as long as we put some examples in MessageDialogOptions.buttons so people can copy paste them, I think the error message wouldn't be a problem in that case

@lucasfernog lucasfernog merged commit 509eba8 into v2 Aug 27, 2025
19 checks passed
@lucasfernog lucasfernog deleted the feat/message-dialog-3-buttons branch August 27, 2025 12:40
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.

Allow for a third button in the MessageDialogBuilder
4 participants