-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
[WIP] React FC conversion for editor file retry + codemirror update #3230
base: develop
Are you sure you want to change the base?
Conversation
Release EnvironmentsThis Environment is provided by Release, learn more! 🔧Environment Status : https://app.release.com/public/Processing%20Foundation/env-fdb9d83e1a |
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.
I saw that you closed my old draft PR #2367. It was not 100% done but there is a lot of good code in there that I hope you can use, particularly with all of the extensions and customizations.
You have to be very careful that any function which you provide to the CodeMirror editor will be called with the right variables at the time of execution. The state management part of working with CodeMirror is a nightmare because it has its own internal state which is handled very differently from React. It looks like the @uiw/react-codemirror
essentially turns the editor into a controlled component so that could help. If you want I can update my WIP to use the @uiw/react-codemirror
package. I'm obviously biased but I think that would make a lot more sense than starting over from scratch since I already tackled a lot of our modifications/enhancements such as the custom search box.
@raclim I wrote 2,000 lines of code which just sat there for over a year with zero feedback. Did anyone even read the code? Or check out and run the branch? Then after a year it just gets unceremoniously closed. Still zero feedback on anything that I wrote. And I see no semblance of a plan of how this conversion will be handled. Is @nahbee10 working on it? Is it a priority?
closeOnUnfocus: false | ||
view.dom.style.fontSize = `${fontSize}px`; | ||
|
||
view.dom.addEventListener('keydown', onKeyDown); |
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.
Any interaction with the DOM should be inside of a useEffect
, where your cleanup function removes the previous listener when adding a new or updated one. Otherwise you will be adding an additional event handler on every render.
Hi @lindapaiste Glad to meet you since I was also closely looking at your code when I'm preparing to work on this feature. |
@nahbee10 I'd guess that I have it around 70% done in that other PR (so you can imagine my frustration). I would recommend that you make a checklist with all of the behaviors and styles which need to be re-implemented when upgrading to v6. In particular: all of the addons that are imported, the addons that we uploaded and modified, the options that we set, and the keyboard shortcuts. Then you can run the branch for my draft and see which ones I have and have not addressed. I left a bunch of |
package.json
Outdated
"@gatsbyjs/webpack-hot-middleware": "^2.25.3", | ||
"@pmmmwh/react-refresh-webpack-plugin": "^0.5.11", | ||
"@redux-devtools/core": "^3.11.0", | ||
"@redux-devtools/dock-monitor": "^3.0.1", | ||
"@redux-devtools/log-monitor": "^4.0.2", | ||
"@reduxjs/toolkit": "^1.9.3", | ||
"@replit/codemirror-css-color-picker": "^6.2.0", | ||
"@simonwep/pickr": "^1.9.1", | ||
"@uiw/react-codemirror": "^4.23.1", |
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.
if we're not using this one let's remove 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.
removed most but left this @replit/codemirror-css-color-picker to see if it is useful to implement the color picker!
|
||
const replaceCommand = metaKey === 'Ctrl' ? `Mod-h` : `Mod-Alt-f`; | ||
|
||
// Handle document changes (debounced) |
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.
i don't see this being debounced? unless i missed something (very likely haha)? but if it's not being debounced lets change the comment or if it should be, let's add the debouncer
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.
yeah thanks for picking this up! I think i need to add back the debouncer. Will update it on the next commit!
- Upgrades jest version to 29.7 - We were getting errors related to the new emmetio import. This is because emmetio formerly compiled to both commonJS and ES modules but now only compileds to ES modules. By default, Babel will ignore node_modules, but Jest does not support ES modules out of the box (only commonJS), so we add a custom transformIgnorePatterns so that Babel knows to transform emmetio to commonJS in the jest environment.
Just wanted to provide a little more info about the test fixes in case anyone is curious! The jest/babel changes are meant to address this error that we were formerly getting:
The problematic import in question was the newly introduced emmetio/codemirror6-plugin, which only compiles to a ECMAScript module, which is not enabled by default in Jest, as specified in the above error message. What we do to fix this is:
I also updated Jest in case that helped since it looks like a lot of new features had been introduced. The other changes in the same CL are just small test error fixes but nothing interesting : ) |
} from '@codemirror/search'; | ||
|
||
import classNames from 'classnames'; | ||
import StackTrace from 'stacktrace-js'; |
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.
This doesn't necessarily have to be addressed in this PR, but wanted to note that stacktrace-js
is pretty outdated now should probably be replaced! Potentially with maybe strack-trace
?
Fixes #issue-number
Changes:
lintMessage
andfontSize
to dependency array for useEffectI have verified that this pull request:
npm run lint
)npm run test
)develop
branch.Fixes #123