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
Merged

Hide undo redo #175

merged 11 commits into from
May 21, 2019

Conversation

alexcjohnson
Copy link
Collaborator

Companion to plotly/dash#724

@alexcjohnson alexcjohnson requested a review from byronz May 20, 2019 23:59
_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

})
);
};
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

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

@@ -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

Copy link
Contributor

@Marc-Andre-Rivet Marc-Andre-Rivet left a comment

Choose a reason for hiding this comment

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

💃

@alexcjohnson alexcjohnson merged commit dd86afc into master May 21, 2019
@alexcjohnson alexcjohnson deleted the hide-undo-redo branch May 21, 2019 17:58
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants