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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Fix new page form clearing when selecting a Under another page option #765

Merged

Conversation

bergice
Copy link
Contributor

@bergice bergice commented Nov 29, 2018

Fixes #2339

Revert d05e142

EDIT:

In the end this didn't revert but instead resolved the issue by creating an empty container for the React loading component to mount inside rather than mounting in a container that already had a potentially significant DOM tree.

@robbieaverill
Copy link
Contributor

Does this mean we lose all the loading animations again? I’d rather have them than fix this bug, if I had to choose

@bergice
Copy link
Contributor Author

bergice commented Nov 29, 2018

Does this mean we lose all the loading animations again? I’d rather have them than fix this bug, if I had to choose

Yep.

On closer look I can see this is caused by 2f2fb2b due to ReactDOM.render clearing the content of the container the component is rendered in:

ReactDOM.render() controls the contents of the container node you pass in. Any existing DOM elements inside are replaced when first called. Later calls use React’s DOM diffing algorithm for efficient updates.
https://reactjs.org/docs/react-dom.html#render

Will update PR.

@bergice bergice self-assigned this Nov 29, 2018
@robbieaverill
Copy link
Contributor

Boom

@bergice bergice force-pushed the pulls/1.3/2339-fix-form-clearing branch from cf21439 to 628a678 Compare November 30, 2018 00:29
@bergice bergice removed their assignment Nov 30, 2018
client/src/legacy/LeftAndMain.js Outdated Show resolved Hide resolved
@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Dec 2, 2018

Ok I literally prepared this exact patch for another issue I've encountered where gridfield-orderable-rows is not working.

@ScopeyNZ
Copy link
Contributor

ScopeyNZ commented Dec 3, 2018

The test failures are pre-existing in 1.3: https://travis-ci.org/silverstripe/silverstripe-admin/builds/462576472

They're being looked at separately.

@ScopeyNZ ScopeyNZ merged commit 9f68956 into silverstripe:1.3 Dec 3, 2018
@maxime-rainville maxime-rainville deleted the pulls/1.3/2339-fix-form-clearing branch December 10, 2018 01:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants