Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Hide undo redo #175

Merged
merged 11 commits into from
May 21, 2019
4 changes: 4 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,10 @@
All notable changes to this project will be documented in this file.
This project adheres to [Semantic Versioning](http://semver.org/).

## [UNRELEASED]
### Changed
- Undo/redo toolbar is removed by default, unless `config.show_undo_redo=true` is provided. The CSS hack `._dash-undo-redo:{display:none;}` is no longer needed [#175](https://github.com/plotly/dash-renderer/pull/175)

## [0.24.0] - 2019-05-15
### Fixed
- Fix regression on handling PreventUpdate (204 NO CONTENT) [#170](https://github.com/plotly/dash-renderer/pull/170)
Expand Down
3 changes: 2 additions & 1 deletion src/AppContainer.react.js
Original file line number Diff line number Diff line change
Expand Up @@ -30,9 +30,10 @@ class UnconnectedAppContainer extends React.Component {
if (type(config) === 'Null') {
return <div className="_dash-loading">Loading...</div>;
}
const {show_undo_redo} = config;
return (
<React.Fragment>
<Toolbar />
{show_undo_redo ? <Toolbar /> : null}
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe a note in the Changelog to the effect that that the CSS solution is now obsolete.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

changelog: e1941e1

<APIController />
<DocumentTitle />
<Loading />
Expand Down
30 changes: 8 additions & 22 deletions src/actions/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -131,34 +131,20 @@ export function redo() {
};
}

const UNDO = createAction('UNDO')();
export function undo() {
return function(dispatch, getState) {
const history = getState().history;
dispatch(createAction('UNDO')());
const previous = history.past[history.past.length - 1];

// Update props
dispatch(
createAction('UNDO_PROP_CHANGE')({
itempath: getState().paths[previous.id],
props: previous.props,
})
);

// Notify observers
dispatch(
notifyObservers({
id: previous.id,
props: previous.props,
})
);
};
return undo_revert(UNDO);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

DRYing this section didn't change the functionality at all... but I discovered I could while researching another question: Can we remove the history from the store entirely when these buttons are hidden? The answer to that is no: in principle we could simplify it to saving just the last past entry and nothing else - which may be a worthwhile improvement for very large and long-running apps, it's possible the memory usage will eventually cause problems. But revert is still needed for error recovery.

Copy link
Contributor

Choose a reason for hiding this comment

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

Could you open an issue to investigate/implement the improvement? Wonder how much of an issue this could be today with long-running apps updating graph/table components.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Could you open an issue to investigate/implement the improvement? Wonder how much of an issue this could be today with long-running apps updating graph/table components.

plotly/dash#727

}

const REVERT = createAction('REVERT')();
export function revert() {
return undo_revert(REVERT);
}

function undo_revert(undo_or_revert) {
return function(dispatch, getState) {
const history = getState().history;
dispatch(createAction('REVERT')());
dispatch(undo_or_revert);
const previous = history.past[history.past.length - 1];

// Update props
Expand Down
2 changes: 1 addition & 1 deletion tests/test_assets/initial_state_dash_app_content.html
Original file line number Diff line number Diff line change
@@ -1 +1 @@
<div id="react-entry-point"><div class="_dash-undo-redo" data-radium="true" style="position: fixed; bottom: 30px; left: 30px; font-size: 20px; text-align: center; z-index: 9999; background-color: rgba(255, 255, 255, 0.9);"><div data-radium="true" style="position: relative;"></div></div><div>Basic string3.14<div id="p.c.4" class="my-class" title="tooltip" style="color: red; font-size: 30px;">Child div with basic string</div><div id="p.c.5"></div><div id="p.c.6"><div id="p.c.6.p.c.0">Grandchild div</div><div id="p.c.6.p.c.1"><div id="p.c.6.p.c.1.p.c.0">Great grandchild</div>3.14159another basic string</div><div id="p.c.6.p.c.2"><div id="p.c.6.p.c.2.p.c.0"><div id="p.c.6.p.c.2.p.c.0.p.c"><div id="p.c.6.p.c.2.p.c.0.p.c.p.c.0"><div id="p.c.6.p.c.2.p.c.0.p.c.p.c.0.p.c.0"></div><div id="p.c.6.p.c.2.p.c.0.p.c.p.c.0.p.c.2"></div></div></div></div></div></div></div></div>
<div id="react-entry-point"><div>Basic string3.14<div id="p.c.4" class="my-class" title="tooltip" style="color: red; font-size: 30px;">Child div with basic string</div><div id="p.c.5"></div><div id="p.c.6"><div id="p.c.6.p.c.0">Grandchild div</div><div id="p.c.6.p.c.1"><div id="p.c.6.p.c.1.p.c.0">Great grandchild</div>3.14159another basic string</div><div id="p.c.6.p.c.2"><div id="p.c.6.p.c.2.p.c.0"><div id="p.c.6.p.c.2.p.c.0.p.c"><div id="p.c.6.p.c.2.p.c.0.p.c.p.c.0"><div id="p.c.6.p.c.2.p.c.0.p.c.p.c.0.p.c.0"></div><div id="p.c.6.p.c.2.p.c.0.p.c.p.c.0.p.c.2"></div></div></div></div></div></div></div></div>
85 changes: 79 additions & 6 deletions tests/test_render.py
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
# -*- coding: UTF-8 -*-
import os
import textwrap

Expand Down Expand Up @@ -109,7 +110,6 @@ def request_queue_assertions(
if expected_length is not None:
self.assertEqual(len(request_queue), expected_length)


def test_initial_state(self):
app = Dash(__name__)
my_class_attrs = {
Expand Down Expand Up @@ -154,11 +154,12 @@ def test_initial_state(self):
self.startServer(app)
el = self.wait_for_element_by_css_selector('#react-entry-point')

# Note: this .html file shows there's no undo/redo button by default
Copy link
Contributor

Choose a reason for hiding this comment

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

Due to visual tests instability I would prefer to isolate this behavior in its own test instead of grafting it to an existing one. Specifically, testing that undo & redo buttons are not found in the DOM + a percy snapshot.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'll include a test that these elements aren't in the DOM. The test I already added includes a percy snapshot with undo AND redo buttons - I guess for completeness I could extend it test that at either end of the history the appropriate button disappears, I'll do that - but we now have a million percy snapshots that used to have undo buttons and no longer do.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

extended element existence tests -> 56cfddc

_dash_app_content_html = os.path.join(
os.path.dirname(__file__),
'test_assets', 'initial_state_dash_app_content.html')
with open(_dash_app_content_html) as fp:
rendered_dom = BeautifulSoup(fp.read(), 'lxml')
rendered_dom = BeautifulSoup(fp.read().strip(), 'lxml')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

strip because my editor doesn't want to save files without a trailing newline

fetched_dom = BeautifulSoup(el.get_attribute('outerHTML'), 'lxml')

self.assertEqual(
Expand Down Expand Up @@ -211,6 +212,79 @@ def test_initial_state(self):

self.assertTrue(self.is_console_clean())

def click_undo(self):
undo_selector = '._dash-undo-redo span:first-child div:last-child'
undo = self.wait_for_element_by_css_selector(undo_selector)
self.wait_for_text_to_equal(undo_selector, 'undo')
undo.click()

def click_redo(self):
redo_selector = '._dash-undo-redo span:last-child div:last-child'
self.wait_for_text_to_equal(redo_selector, 'redo')
redo = self.wait_for_element_by_css_selector(redo_selector)
redo.click()

def check_undo_redo_exist(self, has_undo, has_redo):
selector = '._dash-undo-redo span div:last-child'
els = self.driver.find_elements_by_css_selector(selector)
texts = (['undo'] if has_undo else []) + (['redo'] if has_redo else [])

self.assertEqual(len(els), len(texts))
for el, text in zip(els, texts):
self.assertEqual(el.text, text)

def test_undo_redo(self):
app = Dash(__name__, show_undo_redo=True)
app.layout = html.Div([dcc.Input(id='a'), html.Div(id='b')])

@app.callback(Output('b', 'children'), [Input('a', 'value')])
def set_b(a):
return a

self.startServer(app)

a = self.wait_for_element_by_css_selector('#a')
a.send_keys('xyz')

self.wait_for_text_to_equal('#b', 'xyz')
self.check_undo_redo_exist(True, False)

self.click_undo()
self.wait_for_text_to_equal('#b', 'xy')
self.check_undo_redo_exist(True, True)

self.click_undo()
self.wait_for_text_to_equal('#b', 'x')
self.check_undo_redo_exist(True, True)

self.click_redo()
self.wait_for_text_to_equal('#b', 'xy')
self.check_undo_redo_exist(True, True)

self.percy_snapshot(name='undo-redo')

self.click_undo()
self.click_undo()
self.wait_for_text_to_equal('#b', '')
self.check_undo_redo_exist(False, True)

def test_no_undo_redo(self):
app = Dash(__name__)
app.layout = html.Div([dcc.Input(id='a'), html.Div(id='b')])

@app.callback(Output('b', 'children'), [Input('a', 'value')])
def set_b(a):
return a

self.startServer(app)

a = self.wait_for_element_by_css_selector('#a')
a.send_keys('xyz')

self.wait_for_text_to_equal('#b', 'xyz')
toolbar = self.driver.find_elements_by_css_selector('._dash-undo-redo')
self.assertEqual(len(toolbar), 0)

def test_array_of_falsy_child(self):
app = Dash(__name__)
app.layout = html.Div(id='nully-wrapper', children=[0])
Expand Down Expand Up @@ -328,10 +402,9 @@ def update_input(value):
'#react-entry-point').get_attribute('innerHTML'),
'lxml').select_one('#output > div').contents

self.assertTrue(
pad_input.attrs == {'type': 'text', 'id': 'sub-input-1', 'value': 'sub input initial value'}
and pad_input.name == 'input',
"pad input is correctly rendered")
self.assertEqual(pad_input.attrs['value'], 'sub input initial value')
self.assertEqual(pad_input.attrs['id'], 'sub-input-1')
self.assertEqual(pad_input.name, 'input')

self.assertTrue(
pad_div.text == pad_input.attrs['value']
Expand Down