Isolated JavalabEditor Redux refactor#47445
Merged
bencodeorg merged 20 commits intostagingfrom Aug 3, 2022
Merged
Conversation
sanchitmalhotra126
approved these changes
Aug 2, 2022
Contributor
sanchitmalhotra126
left a comment
There was a problem hiding this comment.
LGTM! I do think it's still worthwhile to merge this change even if we're not immediately following up with further refactors. I don't think it really affects the readability or complexity all that much since all the naming and logic is the same, it's just coming from redux. And with this merged, I feel like picking up further refactor work will be easier down the road. That being said, I've also been working off of this code for a while now so I may be biased :) curious how others feel as well.
molly-moen
approved these changes
Aug 3, 2022
Contributor
molly-moen
left a comment
There was a problem hiding this comment.
LGTM! We should have time to finish this up after summer swarms, and since we've done so much on it already I think it's worth doing :)
Contributor
Author
|
Thanks to you both for the input! |
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 PR breaks off tabs/core editor refactor work (ie, moving state into Redux to enable future breaking up of JavalabEditor). One thing I'd like in this review is a quick check that we still believe that this is a good direction to go in -- I don't know how much space we have to continue breaking up JavalabEditor in our schedule, and this PR does effectively make things a bit more complicated than they were before in the meantime.
Some more details -- this PR isolates commits up to this commit from this PR from other changes that were merged in from Sanchit's refactor work. Changes up to that point were reviewed in that PR.
This PR adds updates to JavalabEditorTest.js, which should be reviewed here.
Testing story
Ran through JavalabEditor unit tests successfully locally.