Dialog: better documentation needed for handling open state

[ccinelli]

Bug Report

eBayUI Version: 2.0.5

Description

class {
    onCreate(input) {
        this.state = {
            selectedDate: false,
            selectedDateActive: false,
        };
    }

    open(opt) {
        this.state.selectedDate = new Date();
        this.state.selectedDateActive = true; // TODO: To link with the real state
        setTimeout(() => {
            this.el.querySelector('.side-dialog').open = true;
        }, 10);
    }

    switch(opts) {
        console.log(opts);
        this.state.selectedDateActive = opts.checked;
    }

    save() {
        //TODO; save
        console.log('Saving', this.state.selectedDate, this.state.selectedDateActive);
        this.el.querySelector('.side-dialog').open = false;
    }
};

<lasso-page flags="ebayui-no-bg-icons"/>

<div>
    <span>Date: ${state.selectedDate}</span>
    <div>
        <ebay-button class="open" priority="primary" on-button-click('open')>Open</ebay-button>
    </div>
    <ebay-dialog class='side-dialog' type="right" a11y-close-text="Close Dialog" aria-labelledby="dialog-title" open=state.dialogOpen>
        <p>Turn the switch please</p>
        <ebay-switch checked=false on-switch-change('switch')/>
        <div>
            <ebay-button class="save" priority="primary" on-button-click('save')>Save & Close</ebay-button>
        </div>
    </ebay-dialog>
</div>

Click on "Open", then click on the "switch". If you click on the switch again the dialog closes when no explicit command to close is issued. The dialog should not close.

[DylanPiercey]

This issue here is that when you render the ebay-dialog in your Marko template the open attribute essentially defaults to false. This means that in this case, when you update your components state it is essentially rerendering the dialog in the closed state.

I notice you are using the property api (el.open = boolean) which is really something we don't recommend when consuming these components with Marko (more meant for when consuming in apps that do not use Marko or use vanilla js).

In this case I'd recommend to store the dialogs open state in your component as you half did in https://github.com/eBay/ebayui-core/issues/582.

The recommended way to use the in Marko dialog would be like so:

class {
    onCreate(input) {
        this.state = {
            dialogOpen: false
            selectedDate: false,
            selectedDateActive: false,
        };
    }

    open(opt) {
        this.state.selectedDate = new Date();
        this.state.selectedDateActive = true; // TODO: To link with the real state
    }

    handleDialogClose() {
      this.state.dialogOpen = false;
    }

    switch(opts) {
        console.log(opts);
        this.state.selectedDateActive = opts.checked;
    }

    save() {
        //TODO; save
        console.log('Saving', this.state.selectedDate, this.state.selectedDateActive);
        this.el.querySelector('.side-dialog').open = false;
    }
};

<lasso-page flags="ebayui-no-bg-icons"/>

<div>
    <span>Date: ${state.selectedDate}</span>
    <div>
        <ebay-button class="open" priority="primary" on-button-click('open')>Open</ebay-button>
    </div>
    <ebay-dialog class='side-dialog' type="right" a11y-close-text="Close Dialog" aria-labelledby="dialog-title" open=state.dialogOpen on-dialog-close('handleDialogClose')>
        <p>Turn the switch please</p>
        <ebay-switch checked=false on-switch-change('switch')/>
        <div>
            <ebay-button class="save" priority="primary" on-button-click('save')>Save & Close</ebay-button>
        </div>
    </ebay-dialog>
</div>

The above adds a dialogOpen state and syncs it with the open attribute. On additional thing to consider is that the user can close the dialog and that you will need to update your state when this happens to keep things consistent. You can see I added an on-dialog-close handler which syncs up your state with the internal dialog state when it is closed.

I think we may just have a documentation issue here.

[ccinelli]

Yes. Documentation issue for sure. It would be nice to have examples in the readme.md of the component but even the https://github.com/eBay/ebayui-core/tree/master/src/components/ebay-dialog/examples does not help.

I also think that the current syntax can be confusing. The dialog has a open input attribute but also an internal state open that get out of sync unless they are used as you explained. I think there are 2 clean solutions:

1. The name should either be initialOpenState or
2. The component should not have a state and delegate the decision on its state to the parent.

If we want to enable both options, it may have an extra attribute (ex: is-stateful) to decide who owns the control of the state and use open or `initialOpenState attribute accordingly.

Probably it is better to make a choice and stick with it.

[ianmcburnie]

Oops, can close this old ticket. We have long since dropped the imperative API.