Skip to content
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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Confirmation modal #211

Merged
merged 17 commits into from Jul 18, 2018

Conversation

Projects
None yet
4 participants
@T4rk1n
Copy link
Contributor

commented Jun 15, 2018

Update July 9, 2018
This component is almost ready! We're still interested in hearing from the community about this component. You can try out this component by installing the "prerelease" version:
pip install dash-core-components==0.25.0rc2
Feedback welcome 馃檶

Simple example:

import dash
from dash.dependencies import Input, Output
import dash_html_components as html
import dash_core_components as dcc

app = dash.Dash()

app.layout = html.Div([
    dcc.ConfirmDialogProvider(
        children=html.Button(
            'Click Me',
        ),
        id='danger-danger',
        message='Danger danger! Are you sure you want to continue?'
    ),
    html.Div(id='output')
])


@app.callback(Output('output', 'children'),
              [Input('danger-danger', 'submit_n_clicks')])
def update_output(submit_n_clicks):
    if not submit_n_clicks:
        return ''
    return """
        It was dangerous but we did it!
        Submitted {} times
    """.format(submit_n_clicks)

if __name__ == '__main__':
    app.run_server(debug=True)

confirm-dialog

Another example with callback controlled confirm dialog:

import dash
from dash.dependencies import Input, Output
import dash_html_components as html
import dash_core_components as dcc

app = dash.Dash()

app.layout = html.Div([
    dcc.ConfirmDialog(
        id='confirm',
        message='Danger danger! Are you sure you want to continue?',
    ),

    dcc.Dropdown(
        options=[
            {'label': i, 'value': i}
            for i in ['Safe', 'Danger!!']
        ],
        id='dropdown'
    ),
    html.Div(id='output')
])


@app.callback(Output('confirm', 'displayed'),
              [Input('dropdown', 'value')])
def display_confirm(value):
    if value == 'Danger!!':
        return True
    return False


@app.callback(Output('output', 'children'),
              [Input('confirm', 'submit_n_clicks')])
def update_output(submit_n_clicks):
    if submit_n_clicks:
        return 'It wasnt easy but we did it {}'.format(submit_n_clicks)


if __name__ == '__main__':
    app.run_server(debug=True)

confirm-dialog-dropdown

@T4rk1n T4rk1n requested review from bpostlethwaite and chriddyp Jun 15, 2018

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Jun 15, 2018

Here's a screenshot of the Confirm modal, percy couldn't take a snapshot of it.
comfirmsnap

@chriddyp

This comment has been minimized.

Copy link
Member

commented Jun 15, 2018

Looking good! I'd like to submit this to the community for review. Could you post a couple of representative code examples in this PR?

@T4rk1n

This comment has been minimized.

Copy link
Contributor Author

commented Jun 18, 2018

Here's an example of dash app where you can add cities to the graph. If you push the reset button, it will ask to confirm before removing the user cities from the graph.

import json
import dash
import dash_core_components as dcc
import dash_html_components as html

from dash.dependencies import Input, Output, State

app = dash.Dash()

app.scripts.config.serve_locally = True
app.css.config.serve_locally = True

data = [
    {
        'x': [32],
        'y': [48],
        'name': 'Washington',
        'mode': 'markers',
        'marker': {'size': 12}
    },
    {
        'x': [12],
        'y': [9],
        'name': 'New York',
        'mode': 'markers',
        'marker': {'size': 12}
    }
]

container_style = {'display':'flex', 'flex-direction': 'column'}

app.layout = html.Div([
    html.Div(id='controls', style=container_style,  children=[
        dcc.Input(id='city-name', placeholder='City name'),
        dcc.Input(id='x', placeholder='x'),
        dcc.Input(id='y', placeholder='y'),
        html.Button(id='add-button', children='add'),
        html.Button(id='reset-button', children='Reset', n_clicks=0),
    ]),
    html.Div([
        dcc.Confirm(id='confirm', message='Warning, you will lose all custom input.'),
        dcc.Graph(id='graph'),
        html.Div(id='values', style={'display': 'None'})
    ]),
], style={'display': 'flex', 'justify-items': 'stretch', 'align-items': 'stretch'})


@app.callback(Output('values', 'children'),
              [Input('add-button', 'n_clicks'), Input('confirm', 'result')],
              [State('city-name', 'value'), State('x', 'value'), State('y', 'value'), State('values', 'children')])
def on_add_city(n_clicks, confirm_result, city_name, x, y, values):
    if confirm_result:
        return ''
    if n_clicks > 0:
        graph_values = json.loads(values) if values else list(data)
        graph_values.append({
            'x': [int(x)],
            'y': [int(y)],
            'name': city_name,
            'mode': 'markers',
            'marker': {'size': 12}
        })
        return json.dumps(graph_values)


@app.callback(Output('confirm', 'init'), [Input('reset-button', 'n_clicks')])
def on_click_confirm(n_clicks):
    if n_clicks > 0:
        return {'ask': True, 'value': 'confirmed'}
    return


@app.callback(Output('graph', 'figure'), [Input('values', 'children')])
def on_change_data(values):
    if values:
        v = json.loads(values)
        return {'data': v}
    return {'data': data}


if __name__ == '__main__':
    app.run_server(debug=True)
@chriddyp

This comment has been minimized.

Copy link
Member

commented Jun 19, 2018

I personally find the nested property structure a little too complex:

  • I'm not sure of the use case for wanting to pass a value through the flow - If the user needs flexible data, they can set data in other inputs and include those as callbacks
  • It's still not quite clear to me what ask does, which might be a sign that we can simplify this
  • I try to avoid nested objects as properties since callbacks target a single top-level property. With nested properties, you have to update all the properties inside the object, you can't just target a single property

As a user, I think I would expect an API something like this:

html.Div([

    Button(
        'Click me',
        id='my-button',
        n_clicks=0
    ),

    ConfirmDialog(
        id='my-confirm-dialog',

        message='''
        Are you sure?
        ''',

        cancel_text='Cancel',
        submit_text='Submit',

        displayed=False,
        
        cancel_n_clicks=0,
        cancel_n_clicks_timestamp=0,

        submit_n_clicks=0,  # or just n_clicks
        submit_n_clicks_timestamp=0,
    ),

    dcc.Input(id='my-input'),
    html.Div(id='my-output')
    
])

@app.callback(Output('my-confirm-dialog', 'displayed'), [Input('my-button', 'n_clicks')])
def display_confirm(n_clicks):
    if n_clicks > 0:
        return True
    return False

    
@app.callback(Output('my-output', 'children'),
              [Input('my-confirm-dialog', 'n_clicks_submit')],
              [State('my-input', 'value')])
def display_confirm(n_clicks, value):
    if n_clicks == 0:
        raise dash.exceptions.PreventUpdate()

    # do something dangerous
    return 'your results are: ...'

A few notes about this API:

  • A clear description of what gets applied to submit vs cancel and consistent naming between those two
  • Consistent naming with the existing click events and timestamps
  • A stateful property designating whether the item is "displayed" with a clear name indicating that state (displayed)

Now, in many cases, this pattern is going always be tied with a button and is always going to have this tedious callback structure that displays the modal when the user clicks the button:

@app.callback(Output('my-confirm-dialog', 'displayed'), [Input('my-button', 'n_clicks')])
def display_confirm(n_clicks):
    if n_clicks > 0:
        return True
    return false

Given that this is going to be really common, I wonder if we should have an additional component that takes a Dash component as children and hijacks the children's click events. Here is what that might look like (ConfirmDialogProvider):

html.Div([

    ConfirmDialogProvider(
        children=Button('Click me'),
    
        id='my-confirm-dialog',

        message='''
        Are you sure?
        ''',

        cancel_text='Cancel',
        submit_text='Submit',

        displayed=False,
        
        cancel_n_clicks=0,
        cancel_n_clicks_timestamp=0,

        submit_n_clicks=0,  # or just n_clicks
        submit_n_clicks_timestamp=0,
    ),

    dcc.Input(id='my-input'),
    html.Div(id='my-output')
    
])

    
@app.callback(Output('my-output', 'children'),
              [Input('my-confirm-dialog', 'n_clicks_submit')],
              [State('my-input', 'value')])
def display_confirm(n_clicks, value):
    if n_clicks == 0:
        raise dash.exceptions.PreventUpdate()

    # do something dangerous
    return 'your results are: ...'

cc @plotly/dash for your review as well

@nicolaskruchten

This comment has been minimized.

Copy link
Member

commented Jun 20, 2018

I'm concerned about these n_clicks checks against 0... Seems like the confirmation would be one-time-use-only in this case, no? How would we support confirmation every time?

@T4rk1n T4rk1n force-pushed the confirmationModal branch 2 times, most recently from bf68532 to 14bc6e1 Jun 20, 2018

@T4rk1n T4rk1n force-pushed the confirmationModal branch from 14bc6e1 to 86d86e2 Jun 20, 2018

@chriddyp

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

I'm concerned about these n_clicks checks against 0

These checks against are 0 are just for Dash's warm-up initialization, when it fires all of the callbacks on page load.

button.click()
time.sleep(1)

self.driver.switch_to.alert.accept()

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

馃帀 very nice!

/**
* Number of times the modal was submited or canceled.
*/
n_clicks: PropTypes.number,

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

Given that we have submit_n_clicks and cancel_n_clicks, is there a use case for the general n_clicks?

/**
* Set to true to send the popup.
*/
send_confirm: PropTypes.bool,

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

For some reason, "send" doesn't feel right to me. What about display_confirm? Or maybe just displayed? Or display_confirm_dialog? Any other thoughts @plotly/dash ?

/**
* Is the modal currently displayed.
*/
displayed: PropTypes.bool,

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

I think that I thought that displayed would take the place of send_confirm:

@app.callback(Output('confirm', 'displayed'), [Input('my-button', 'n_clicks'])
def display_modal(n_clicks):
    return boolean(n_clicks)

Does that work? Or am I missing a functional difference between displayed and send_confirm?

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Jun 22, 2018

Author Contributor

Send_confirm is for activating the modal, it get sets to false after fire, displayed was for telling if the confirmation was currently showing. I just changed for just displayed and it works the same.

This comment has been minimized.

Copy link
@nicolaskruchten

nicolaskruchten Jun 26, 2018

Member

I think a good rule to keep in mind here and with most functional/React style coding is that prop names should not be verbs, especially not imperative ones like send but rather descriptions of state like displayed :)

import {Component} from 'react';

/**
* ConfirmDialog wraps window.confirm

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

This is the message that will appear in the component's help(dcc.ConfirmDialog), so let's expand this a bit. The Dash users probably don't know what window.confirm is referring to. Perhaps something like:

ConfirmDialog is used to display the browser's native "confirm" modal, with an optional message and two buttons ("OK" and "Cancel"). This ConfirmDialog can be used in conjunction with buttons when the user is performing an action that should require an extra step of verification.

And then similarly for the ConfirmDialogProvider. For the ConfirmDialogProvider, we should mention that you can pass in a button directly as children

const { id, setProps, children } = this.props;

// Will lose the previous onClick of the child
const wrapClick = (child) => React.cloneElement(child, {onClick: () =>

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

Nice, this seems like a good solution.

time.sleep(0.5)
self.wait_for_text_to_equal('#confirmed', 'canceled')

self.snapshot('confirmation -> canceled')

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jun 22, 2018

Member

Very nice tests! I'd like to see a couple of other things:

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Jun 26, 2018

Author Contributor

I added a test for the provider and put a call count variable. The call_count get increased normally in my local tests but in circleci it get increased by 6 instead of two, I put the value to be the n_clicks instead for the tests to work on circleci but I think there's an error.

@chriddyp

This comment has been minimized.

Copy link
Member

commented Jun 22, 2018

Looking good @T4rk1n ! I just made a few other comments. Let's also make a prerelease of this package so that some community members can easily take it for a spin. I wrote some instructions on how to publish prereleases in this comment here: #215 (comment)

@T4rk1n T4rk1n force-pushed the confirmationModal branch from 21c9079 to ab17240 Jun 22, 2018

@T4rk1n T4rk1n force-pushed the confirmationModal branch from ab17240 to d4adc7a Jun 22, 2018

@chriddyp
Copy link
Member

left a comment

Overall this looks good! Just a few comments around the docstrings, adding timestamp to submit and cancel. I'm 馃憤 with this once those changes are made

馃拑

/**
* Wrap children onClick to send a confirmation dialog.
* You can add a button directly as a children:
* `dcc.ConfirmDialogProvider(html.Button('click me', id='btn'), id='confirm')`

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jul 10, 2018

Member

Our users probably won't know what onClick means, so let's change this to:

A wrapper component that will display a confirmation dialog when its child component has been clicked on.
For example:

dcc.ConfirmDialogProvider(
    children=html.Button('Click Me'),
    message='Danger - Are you sure you want to continue?',
    id='confirm'
)
/**
* Number of times the popup was canceled.
*/
cancel_n_clicks: PropTypes.number,

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jul 10, 2018

Member

Can we add submit_n_clicks_timestamp and cancel_n_clicks_timestamp here and in the ConfirmDialog component?

/**
* Number of times the modal was submited or canceled.
*/
n_clicks: PropTypes.number,

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jul 10, 2018

Member

I'm still wondering if there is a use case for this property given that we have submit_n_clicks and cancel_n_clicks. Thoughts @plotly/dash ?

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Jul 10, 2018

Author Contributor

If I add the timestamps for submit and cancel, I don't see one as I was only using it to know which one was clicked in the test and it wasn't a real use case. I will remove it.

*/
n_clicks_timestamp: PropTypes.number,
/**
* Number of times the submit was clicked

This comment has been minimized.

Copy link
@chriddyp

chriddyp Jul 10, 2018

Member

Number of times the submit button was clicked

@bpostlethwaite

This comment has been minimized.

Copy link
Member

commented Jul 18, 2018

@T4rk1n you have the 馃拑 to merge this 鉁旓笍

We have interested parties waiting for release. Thanks!

@T4rk1n T4rk1n force-pushed the confirmationModal branch from 6592fff to ac90a45 Jul 18, 2018

T4rk1n added some commits Jul 18, 2018

Merge branch 'master' into confirmationModal
# Conflicts:
#	CHANGELOG.md
#	test/test_integration.py

@T4rk1n T4rk1n merged commit dba57a8 into master Jul 18, 2018

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
percy/dash-core-components No visual changes since last approval
Details

@T4rk1n T4rk1n deleted the confirmationModal branch Jul 23, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can鈥檛 perform that action at this time.