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
Merged
Changes from 11 commits
Commits
File filter...
Filter file types
Jump to鈥
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -8,6 +8,8 @@ machine:

dependencies:
override:
- pip install setuptools --upgrade
- pip install virtualenv
- pip install tox
- npm install -g eslint
- npm install --ignore-scripts
@@ -0,0 +1,77 @@
import PropTypes from 'prop-types';
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

*/
export default class ConfirmDialog extends Component {

constructor(props) {
super(props);
}

componentDidUpdate() {
const { displayed, message, send_confirm, setProps, cancel_n_clicks, submit_n_clicks, n_clicks } = this.props;

if (send_confirm && !displayed) {
setProps({send_confirm: false, displayed: true});
new Promise(resolve => resolve(window.confirm(message))).then(result => setProps({
n_clicks: n_clicks + 1,
n_clicks_timestamp: Date.now(),
cancel_n_clicks: !result ? cancel_n_clicks + 1 : cancel_n_clicks,
submit_n_clicks: result ? submit_n_clicks + 1: submit_n_clicks,
displayed: false,
}));
}
}

render() {
return null;
}
}

ConfirmDialog.defaultProps = {
n_clicks: 0,
n_clicks_timestamp: -1,
submit_n_clicks: 0,
cancel_n_clicks: 0,
};

ConfirmDialog.propTypes = {
id: PropTypes.string,

/**
* Message to show in the popup.
*/
message: PropTypes.string,

/**
* 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?

/**
* Last timestamp the popup was clicked.
*/
n_clicks_timestamp: PropTypes.number,
/**
* Number of times the submit was clicked
*/
submit_n_clicks: PropTypes.number,
/**
* Number of times the popup was canceled.
*/
cancel_n_clicks: PropTypes.number,
/**
* 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 :)


/**
* Dash-assigned callback that gets fired when the value changes.
*/
setProps: PropTypes.func
};
@@ -0,0 +1,86 @@
import React from 'react';
import PropTypes from 'prop-types';

import ConfirmDialog from './ConfirmDialog.react'



/**
* Wrap children onClick to send a confirmation dialog.
*/
export default class ConfirmDialogProvider extends React.Component {
render() {
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.

{
setProps({
send_confirm: true
});
}
});

const realChild = children.props
? children.props.children
: children.map(e => e.props.children);

return (
<div id={id}>
{
realChild && realChild.length
? realChild.map(wrapClick)
: wrapClick(realChild)
}
<ConfirmDialog {...this.props}/>
</div>
)
}
};

ConfirmDialogProvider.defaultProps = {
n_clicks: 0,
n_clicks_timestamp: -1,
submit_n_clicks: 0,
cancel_n_clicks: 0,
};

ConfirmDialogProvider.propTypes = {
id: PropTypes.string,

/**
* Message to show in the popup.
*/
message: PropTypes.string,

/**
* 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.

/**
* Last timestamp the popup was clicked.
*/
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

*/
submit_n_clicks: PropTypes.number,
/**
* 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?

/**
* Set to true to send the popup.
*/
send_confirm: PropTypes.bool,
/**
* Is the modal currently displayed.
*/
displayed: PropTypes.bool,

/**
* Dash-assigned callback that gets fired when the value changes.
*/
setProps: PropTypes.func,
children: PropTypes.any,
};
@@ -15,9 +15,13 @@ import Textarea from './components/Textarea.react';
import DatePickerSingle from './components/DatePickerSingle.react';
import DatePickerRange from './components/DatePickerRange.react';
import Upload from './components/Upload.react';
import ConfirmDialog from './components/ConfirmDialog.react';
import ConfirmDialogProvider from './components/ConfirmDialogProvider.react'

export {
Checklist,
ConfirmDialog,
ConfirmDialogProvider,
Dropdown,
Graph,
Input,
@@ -4,9 +4,15 @@
import time
import unittest
import percy
import threading
import platform
import flask
import requests

from selenium import webdriver
from selenium.webdriver.chrome.options import Options


class IntegrationTests(unittest.TestCase):

@classmethod
@@ -34,10 +40,19 @@ def setUp(self):

def tearDown(self):
time.sleep(3)
self.server_process.terminate()
if platform.system() == 'Windows':
requests.get('http://localhost:8050/stop')
else:
self.server_process.terminate()
time.sleep(3)

def startServer(self, app):
"""
:param app:
:type app: dash.Dash
:return:
"""
if 'DASH_TEST_PROCESSES' in os.environ:
processes = int(os.environ['DASH_TEST_PROCESSES'])
else:
@@ -52,9 +67,32 @@ def run():
processes=processes
)

def run_windows():
app.scripts.config.serve_locally = True
app.css.config.serve_locally = True

@app.server.route('/stop')
def _stop_server_windows():
stopper = flask.request.environ['werkzeug.server.shutdown']
stopper()
return 'stop'

app.run_server(
port=8050,
debug=False,
threaded=True
)

# Run on a separate process so that it doesn't block
self.server_process = multiprocessing.Process(target=run)
self.server_process.start()

system = platform.system()
if system == 'Windows':
# multiprocess can't pickle an inner func on windows (closure are not serializable by default on windows)
self.server_thread = threading.Thread(target=run_windows)
self.server_thread.start()
else:
self.server_process = multiprocessing.Process(target=run)
self.server_process.start()
time.sleep(5)

# Visit the dash page
@@ -15,8 +15,9 @@
from selenium import webdriver
from selenium.webdriver.common.keys import Keys
from selenium.common.exceptions import InvalidElementStateException
import time

from textwrap import dedent

try:
from urlparse import urlparse
except ImportError:
@@ -366,7 +367,6 @@ def test_gallery(self):

self.snapshot('gallery - text input')


def test_location_link(self):
app = dash.Dash(__name__)

@@ -528,3 +528,48 @@ def update_graph(n_clicks):
button.click()
time.sleep(2)
self.snapshot('candlestick - 2 click')

def test_confirm(self):
app = dash.Dash(__name__)

app.layout = html.Div([
html.Button(id='button', children='Send confirm', n_clicks=0),
dcc.ConfirmDialog(id='confirm', message='Please confirm.'),
html.Div(id='confirmed')
])

@app.callback(Output('confirm', 'send_confirm'), [Input('button', 'n_clicks')])
def on_click_confirm(n_clicks):
if n_clicks:
return True

@app.callback(Output('confirmed', 'children'),
[Input('confirm', 'n_clicks'), Input('confirm', 'submit_n_clicks'), Input('confirm', 'cancel_n_clicks')],)
def on_confirmed(n_clicks, submit_n_clicks, cancel_n_clicks):
if not n_clicks:
return
if n_clicks == 1:
return 'confirmed'
elif n_clicks == 2:
return 'canceled'

self.startServer(app)
button = self.wait_for_element_by_css_selector('#button')
self.snapshot('confirmation -> initial')
time.sleep(1)
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!

self.wait_for_text_to_equal('#confirmed', 'confirmed')

self.snapshot('confirmation -> confirmed')

time.sleep(0.2)
button.click()
time.sleep(1)
self.driver.switch_to.alert.dismiss()
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.

ProTip! Use n and p to navigate between commits in a pull request.
You can鈥檛 perform that action at this time.