Skip to content

[AMP Stories] Re-render components when switching back from code editor#2117

Merged
swissspidy merged 4 commits intoamp-stories-reduxfrom
amp-story/code-editor
Apr 29, 2019
Merged

[AMP Stories] Re-render components when switching back from code editor#2117
swissspidy merged 4 commits intoamp-stories-reduxfrom
amp-story/code-editor

Conversation

@swissspidy
Copy link
Copy Markdown
Collaborator

When working on the default templates I was annoyed that the visual editor was kinda broken when switching back from the code editor.

The reason is that many of the custom DOM nodes get lost in that process.

This PR re-renders the components when switching back and forth between the two modes. It's admittedly a bit hacky, because it needs to wait for the editor to finish render first. I haven't found a better solution yet.

@googlebot googlebot added the cla: yes Signed the Google CLA label Apr 12, 2019
@swissspidy
Copy link
Copy Markdown
Collaborator Author

Ugh. Looks like a ! document.getElementById(...) check is also needed for some of the other components.

@kienstra
Copy link
Copy Markdown
Contributor

kienstra commented Apr 12, 2019

Approved

Hi @swissspidy,
This looks good, and switching back to the Visual Editor works well now.

Maybe this would be pending the ! document.getElementById(...) checks you mentioned, but it looks good so far.

visual-editor-switching

@westonruter
Copy link
Copy Markdown
Member

This is a very welcome change. I was noticing the UI getting blown away when switching to the code editor and back as well.

@swissspidy Let us know when you've addressed the other issue you noted.

@swissspidy swissspidy merged commit 6d88e24 into amp-stories-redux Apr 29, 2019
@swissspidy swissspidy deleted the amp-story/code-editor branch April 29, 2019 09:57
@westonruter westonruter added this to the v1.2 milestone May 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

cla: yes Signed the Google CLA

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants