Skip to content

Conversation

@aduth
Copy link
Member

@aduth aduth commented Nov 15, 2017

Related: #3208 (comment)

This pull request seeks to ensure that editor settings are respected when attempting to reboot from a crashed state. See #3208 (comment) for more context.

Testing instructions:

Ideally there is no way to create a crash in the application. You may introduce one though with the following diff:

diff --git a/components/dropdown/index.js b/components/dropdown/index.js
index 1989f101..1c79f148 100644
--- a/components/dropdown/index.js
+++ b/components/dropdown/index.js
@@ -61,6 +61,7 @@ class Dropdown extends Component {
 
        render() {
                const { isOpen } = this.state;
+               if ( isOpen ) throw new Error();
                const { renderContent, renderToggle, position = 'bottom', className, contentClassName } = this.props;
                const args = { isOpen, onToggle: this.toggle, onClose: this.close };
                return (

After applying these changes, add a breakpoint in the recreateEditorInstance function, incur the crash by opening a dropdown (e.g. inserter menu), then "Attempt Restore" and verify that recreateEditorInstance is called with the original settings of the editor.

@aduth aduth added the Framework Issues related to broader framework topics, especially as it relates to javascript label Nov 15, 2017
@aduth aduth requested a review from gziolo November 15, 2017 19:55
@codecov
Copy link

codecov bot commented Nov 15, 2017

Codecov Report

Merging #3500 into master will not change coverage.
The diff coverage is 0%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #3500   +/-   ##
=======================================
  Coverage   34.61%   34.61%           
=======================================
  Files         260      260           
  Lines        6757     6757           
  Branches     1226     1226           
=======================================
  Hits         2339     2339           
  Misses       3728     3728           
  Partials      690      690
Impacted Files Coverage Δ
editor/index.js 0% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5255c93...46cd3bb. Read the comment docs.

editor/index.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

Setting needs to be passed here, too

Copy link
Member

Choose a reason for hiding this comment

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

‘settings’ - I’m on mobile 😃

Copy link
Member Author

Choose a reason for hiding this comment

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

febf5d9

@aduth aduth force-pushed the fix/editor-provider-reboot-settings branch from febf5d9 to 46cd3bb Compare November 15, 2017 20:27
@gziolo
Copy link
Member

gziolo commented Nov 16, 2017

Thanks for fixing this potential issue 👍

@gziolo gziolo merged commit afd2627 into master Nov 16, 2017
@gziolo gziolo deleted the fix/editor-provider-reboot-settings branch November 16, 2017 07:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Framework Issues related to broader framework topics, especially as it relates to javascript

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants