JavalabEditor refactor: move editor state into Redux#46970
Merged
sanchitmalhotra126 merged 23 commits intostagingfrom Aug 8, 2022
Merged
JavalabEditor refactor: move editor state into Redux#46970sanchitmalhotra126 merged 23 commits intostagingfrom
sanchitmalhotra126 merged 23 commits intostagingfrom
Conversation
molly-moen
reviewed
Jun 23, 2022
|
|
||
| export const getTabKey = index => `file-${index}`; | ||
|
|
||
| const fileMetadataForEditor = (sources, isEditingStartSources) => { |
Contributor
There was a problem hiding this comment.
could this logic of converting sources -> fileMetadata be done in some helper function elsewhere (that is called before setting fileMetadata in redux)? Generally I think we want to keep redux free of complex logic.
Contributor
Author
There was a problem hiding this comment.
Ok makes sense -- I stumbled across this file yesterday, might be a good place for it:
https://github.com/code-dot-org/code-dot-org/blob/staging/apps/src/javalab/JavalabFileHelper.js
8 tasks
8 tasks
sanchitmalhotra126
approved these changes
Jul 18, 2022
Contributor
sanchitmalhotra126
left a comment
There was a problem hiding this comment.
LGTM! Like we discussed I think these changes will be really helpful in moving more stuff out of this file down the road.
…factor/file-errors-redux Move file error message into redux
…factor/split-dialog-header Move Javalab Editor header and dialogs into their own components
…factor/commitdialog Move compile status state management into CommitDialog
Move dialog state into redux
8 tasks
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
3 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.
This is the "things get worse before they get better" PR -- moves most of the state needed to break up JavalabEditor into Redux.
I think I'm not going to merge this PR, because I'd have to fix a bunch of JavalabEditor tests that are going to be updated when this new Redux state is moved into other components (the idea here is to split JavalabEditor up, and I think that a lot of this state will move out of JavalabEditor eventually). I (or someone else) can branch off of this PR to implement some of the splitting up proposed here and here.
I'd appreciate feedback on whether the direction here seems reasonable though!
Links
Testing story
I tested manually that I was able to perform Javalab editor actions (rename/create/delete files, navigate between tabs, import from the backpack) in three different scenarios -- 1) a new project via /projects/javalab/new, 2) an existing level via /allthethings, and 3) editing start sources as a levelbuilder. There should be no new functionality added here, so I did not add any new tests.