feat(lab): new component ConfirmationDialog

[hansnowak]

First, very rough, version of ConfirmationDialog.

[maartenbreddels]

Great to see this Hans. For code quality checks I recommend you follow the pre-commit steps as suggested in https://solara.dev/docs/development . This will run some linters when you commit for a faster feedback loop.

[maartenbreddels]

Hi Hans, I did a first pass. Generally, it looks good 👍 , I'm a bit uncertain about the naming (especially is_open_rv)

[maartenbreddels]

I think we should keep using the pattern as in https://github.com/widgetti/solara/pull/185/files#diff-fb21e018d759a3a11489bdab98c0bd0b7841a772f45ba556178a82c39db6eec1R12

open is a bool or reactive variable, and locally convert it to a reactive variable (if not already) using use_reactive:
https://github.com/widgetti/solara/pull/185/files#diff-fb21e018d759a3a11489bdab98c0bd0b7841a772f45ba556178a82c39db6eec1R54

Then we also need a callback. Maybe we should call it is_open and on_is_open, or just open and on_open, or even value and on_value.

@mariobuikhuizen and @iisakkirotko opinions on naming are appreciated :)

[hansnowak]

Hm, value seems a bit too vague in my opinion... that makes sense for a switch etc, but maybe less so for a dialog... what is the "value" of a dialog?? It might be better to use something with "open". is_open seems to be more in line with what is usually used in Python, indicating a boolean.

I'm not sure what the callback is for? Is it called when is_open (or whatever we end up calling it) changes value?

[maartenbreddels]

for consistency, call it on_ok?

[hansnowak]

OK! :) The other one should probably be on_cancel then...

[hansnowak]

Or does on_cancel or on_close (as it is currently called) become redundant?

[maartenbreddels]

I see an issue here is that a user provided button will not trigger perform_action. It would be nice to expose this during the tests. It might be that if we need to use use_event for this that we hit this bug: widgetti/reacton#14

[hansnowak]

If a user provides their own Button, shouldn't it also be linked up to an action already? Like

def my_cool_action():
    ...
ConfirmationDialog(..., ok=Button(label="Yeah", on_click=my_cool_action), ...)

Then again we currently have on_ok (or on_action) as a required argument, which kind of clashes with that... 🤔

[maartenbreddels]

Yes, as I mentioned in our call, if you write a test we can find a solution to this later on, I have some ideas about it.

[maartenbreddels]

In theory, we should be able to use solara.v.use_event, but it has issues due to widgetti/reacton#14

I think we can do a different approach, which is to inspect ok.kwargs["on_click"] and replace it by a new dict where on_click points to our perform_action, and the perform_action also calls the previous on_click set by the user.

[hansnowak]

So, if I understand this correctly, if you pass a custom Button named ok, then ok.kwargs['on_click'] is only present if you passed an on_click event to your button. Should we allow for a case where a user passes in a custom button that has no on_click? Perhaps because they just want to execute on_ok as intended.

Related: what do we do in the case that both on_ok and a custom button's on_click are set?

[hansnowak]

For context, I currently have this:

    with solara.CardActions():
        if isinstance(ok, str):
            solara.Button(label=ok, on_click=perform_action)
        else:
            solara.display(ok)
            if ok.kwargs.get('on_click'):
                ok.kwargs['on_click'] = perform_action

...which passes the test. I think there is no point in checking ok.kwargs if ok is a string?

[maartenbreddels]

Yes, I think we should support no on_click as well. I suggest we also don't modify the dict, but create a new one.

[hansnowak]

In theory, we should be able to use solara.v.use_event, but it has issues due to widgetti/reacton#14

I think we can do a different approach, which is to inspect ok.kwargs["on_click"] and replace it by a new dict where on_click points to our perform_action, and the perform_action also calls the previous on_click set by the user.

I should have read this more carefully, you're literally describing what to do in a situation with both on_ok and on_click being present.

[maartenbreddels]

This is released in v1.20.0
Thank you Hans!

[icduck]

This is a nice component. Before this, I used v.Dialog which is good too. this ConfirmationDialog would be great if it supports content_class just like v.Dialog and draggable feature. Currently, I adapted this vuetifyjs/vuetify#4058 for draggable dialog in Solara. Love to see it as built-in feature for dialog, just like griddraggable.