-
Notifications
You must be signed in to change notification settings - Fork 35
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
enable editor changes in sketch mode, refactor some of the code manager #2287
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
d27c80f
to
a7b6802
Compare
my bet is fefbd75 will break a bunch of the onboarding and a few other things, but I think that's just because |
@@ -12,7 +12,7 @@ export default function FutureWork() { | |||
|
|||
useEffect(() => { | |||
// We do want to update both the state and editor here. | |||
codeManager.updateCodeStateEditor(bracket) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wait so im confused you dont want to update the state for these? if that state is now never used we can nukke it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
well this isn't my code so not super sure, but there were onboarding tests that failed before this change that are now passing so I think this is what we want.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updateCodeEditor is still used in a couple of more places so I'll keep it for now.
7e0e5be
to
7a8a4db
Compare
1dd2519
to
0029947
Compare
9ee3546
to
131746a
Compare
…ges to t… (#2286)"
This reverts commit e7ab645.
implementing #2283 again, but just making sure playwright passes before merging.
Will also revolve #1558
Screenshare.-.2024-05-03.9_14_00.PM.mp4