[Master] Split BrowserModalComponent out of GUI#2993
Closed
Conversation
Contributor
Author
|
Try it out: hotfix/fix-browser-modal I'm a bot, not actually @rschamp! |
Contributor
|
@chrisgarrity Now it's up to you! |
chrisgarrity
approved these changes
Aug 28, 2018
Contributor
chrisgarrity
left a comment
chrisgarrity
left a comment
There was a problem hiding this comment.
A couple of nitpicky details that would be good to clean up. Otherwise it's ugly, but does the trick.
src/components/gui/gui.jsx
Outdated
| > | ||
| {previewInfoVisible ? ( | ||
| <PreviewModal hideIntro={hideIntro} /> | ||
| {(previewInfoVisible && !hideIntro) ? ( |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
| * @returns {React.Component} component with redux and intl state provided | ||
| */ | ||
| const AppStateHOC = function (WrappedComponent) { | ||
| const AppStateHOC = function (WrappedComponent, localesOnly) { |
This comment was marked as abuse.
This comment was marked as abuse.
Sorry, something went wrong.
Contributor
|
By the way I also verified the unsupported browser on Opera (OSX) |
If the VM is ever instantiated, which happens whenever GUI is instantiated due to default props, then IE11 will crash
rschamp
force-pushed
the
hotfix/fix-browser-modal
branch
from
884e419 to
58b2b76
Compare
added 2 commits
September 5, 2018 11:52
Removing this was the point of moving the test up one level. Thanks @chrisgarrity, good catch!
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I know this is pretty ugly, but I think what we want to avoid crashing unsupported browsers. Available for QA here: https://llk.github.io/scratch-gui/hotfix/fix-browser-modal/
Resolves
What Github issue does this resolve (please include link)?
Resolves failing smoke test in #2980 (already merged). Should future-proof us from this type of regression in the future (Hello from the past future us! Sorry!)
Proposed Changes
Describe what this Pull Request does
This splits the BrowserModalComponent out of the GUI, and protects all GUI-related modules from being imported unless the GUI is actually being mounted. This protects unsupported browsers from running code that will crash them (like the VM).
Reason for Changes
Explain why these changes should be made
If the VM is ever instantiated, which happens whenever GUI is instantiated due to default props, then IE11 will crash. So mount the BrowserModalComponent in a separate code path from the GUI, so that it can be displayed regardless of the code required by the GUI.
Test Coverage
Please show how you have added tests to cover your changes
Failing smoke test now passes!
Browser Coverage
Check the OS/browser combinations tested (At least 2)
Mac
Windows
Chromebook
iPad
Android Tablet