Skip to content
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

[JIMT][CT-482] - fixed editor jumping #57867

Merged
merged 2 commits into from Apr 8, 2024
Merged

Conversation

thomasoniii
Copy link
Contributor

CodeBridge has an issue where any typing would jump the position of the cursor back to the start.

I traced it back to the lab2/CodeEditor component itself, and specifically the second useEffect hook. What's happening is that the startCode value is changing because something new is handed in (this is part of the external loop where the editor caused a change, which fires off an action, which updates state, which eventually circles back to the component itself with a "new" file that's only what the user had just typed.

Fix is easy - in the effect, just compare the current value of the editor's doc and only set it to the new one if it's something different.

Side note - the original version of this fix operated differently. Instead of checking to see if the values had changed, it would just set the new string and reset the selection to what it was previously. That worked, but was concerning because if the file itself actually changed (e.g., we have the same editor open and were working with File A, and have now switched to File B), we could end up with the cursor in a random position in the new file. The way the code exists now, an external file change would always reset the cursor position back to the start, which is probably what we want.

One odd minor edge case could be if the file contents change, but it's still the same one. For example, if we format the file. We'll still reset the cursor position back to the beginning of the file, even though that may not be what the user would like. Ideally, the cursor would stay in the same position in the modified file. AFAIK, fixing that edge case would not be trivial in the general case and I think we should ignore it. This potential edge case is much smaller surface area than the original one with preserving the selection.

Comment on lines +29 to +34
[file.id, saveFile]
);

const editorConfigExtensions = useMemo(
() => [codeMirrorLangMapping[file.language]],
[file.language]
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just some minor memoization tweaks to ensure that repackaged props aren't handed down to the editor

@thomasoniii thomasoniii requested a review from a team April 8, 2024 19:32
Copy link
Contributor

@molly-moen molly-moen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix!

@thomasoniii thomasoniii merged commit a21c667 into staging Apr 8, 2024
2 checks passed
@thomasoniii thomasoniii deleted the jimt/CT-482/editor-jumping branch April 8, 2024 21:09
@thomasoniii thomasoniii requested a review from a team April 11, 2024 17:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants