Remove the hideIntro override and default preview modal to false #3768
Conversation
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
paulkaplan
force-pushed
the
fix-preview-modal
branch
from
9478fc6 to
7d00d3d
Compare
benjiwheeler
suggested changes
Nov 14, 2018
Contributor
benjiwheeler
left a comment
benjiwheeler
left a comment
There was a problem hiding this comment.
Mostly looks good! I'm hoping that the logic can be clarified a bit... LMK if the direction I'm suggesting makes any sense.
Contributor
Author
|
@benjiwheeler thanks for the comments, restructuring the negative "if" is definitely more clear. I don't love the mini reducers in GUI either, but I do not really see a better way to have the initial state be something other than what the reducer says it is. It doesn't really make sense to dispatch to make this change, because there is no user action that is happening, it is just that the initial state is different between gui/www. |
Contributor
|
This looks good, but could you wait for ChrisG to merge the much simpler #3725 , which also touches hideIntro? Should land today |
The use of an "override" prop that allowed the preview modal to be hidden without actually changing the redux state was causing a problem where the block editors where not closing when modals were opening. That is because it relied on the modal reducer being accurate. I defaulted the preview state to false to make it simpler to consume the GUI, as www will not want the preview info to be true. I made it true in the playground instead. This fixes scratchfoundation#3710
paulkaplan
force-pushed
the
fix-preview-modal
branch
from
67d06cf to
3f1666c
Compare
Contributor
|
LGTM |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
The use of an "override" prop that allowed the preview modal to be hidden without actually changing the redux state was causing a problem where the block editors where not closing when modals were opening. That is because it relied on the modal reducer being accurate.
I defaulted the preview state to false to make it simpler to consume the GUI, as www will not want the preview info to be true. I made it true in the playground instead.
This fixes #3710
I can't write an integration test for that specific issue yet because it only happens when embedded, but the current integration tests ensure that the preview modal is still there in the playground the way we want. I confirmed the fix for #3710 by manually linking and testing.