-
Notifications
You must be signed in to change notification settings - Fork 527
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
Replace editor <textarea>
with CodeMirror v6
#2866
Conversation
Rebased to not include other PR contents. Got tests to pass, though needed to skip a couple due to the difference in selection handling. Will need to work on that to help ensure that we also fix #2781. |
Key shortcuts and default selection should now work. This now fixes #2781. |
I'll stop rebasing this now, as it's starting to look properly reviewable |
@@ -10,7 +10,7 @@ import css from 'rollup-plugin-css-only'; | |||
/** @type {import('rollup').RollupOptions} */ | |||
const config = { | |||
input: 'src/index.tsx', | |||
output: { file: 'dist/translate.js' }, | |||
output: { file: 'dist/translate.js', format: 'iife' }, |
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.
One of the new dependencies includes a module with a top-level statement const top = ...
. With the default output.format
, Rollup presumes that the code will be included in a <script type=module>
so leaves it at the top level. However, as we don't do that, the output needs to be wrapped in an IIFE.
|
||
const [state, setState] = useState<EditorData>(initEditorData); | ||
const [state, setState] = useState(initEditorData); | ||
const [result, setResult] = useState<EditorResult>([]); | ||
|
||
const actions = useMemo<EditorActions>(() => { |
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.
The actions that directly set a new value for fields
need to also update result
. Those that call field.handle.current.setValue()
don't, as the EditField and EditAccessKey will take care of that.
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.
Contents refactored & moved to translate/src/modules/translationform/utils/editFieldShortcuts.ts
.
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.
textarea
styles refactored & moved to translate/src/modules/translationform/components/TranslationForm.css
const locale = useContext(Locale); | ||
const { setEditorFromInput } = useContext(EditorActions); | ||
const message = useEditorValue(); | ||
function useHandleShortcuts(): (event: React.KeyboardEvent) => void { |
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.
CodeMirror has its own way of specifying key handlers, so this is now specific to EditAccesskey.
As a follow-up change, we should probably move these handlers out of the editors, so that they're applied also when the focus isn't on the input field, rather like Alt/Option+ArrowDown/ArrowUp
.
cursor: default; | ||
} | ||
|
||
.cm-editor { |
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.
CodeMirror packages in some styling for itself via CSS-in-JS; these are overrides on those defaults. It does include its own styling system, but relying on CSS is just as valid, and more in line with what we've been doing so far.
labels: Array<{ label: string; plural: boolean }>; | ||
}) => ( | ||
<label htmlFor={htmlFor}> | ||
<label> |
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.
Sadly, HTML <label>
and contentEditable
doesn't really play well together, so clicking on the label will no longer focus the appropriate field.
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.
That's a nasty UX issue. Why do we need contenteditable
?
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.
Because it's used by CodeMirror, and effectively the basis on which any solution other than <textarea>
is built.
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.
Oh, ok - I thought we were also using contenteditable
in the label.
userInput={userInput} | ||
value={value[0]?.value} | ||
/> | ||
return pk !== entity.pk ? null : fields.length === 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.
Sometimes, we render TranslationForm before EditorProvider updates on entity change. This pk
check ensures that we don't try to build a form with intermediate data, because that gets pretty messy.
bracketMatching(), | ||
closeBrackets(), | ||
EditorView.lineWrapping, | ||
keymap.of([ |
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.
The first binding that matches and returns true
stops further bindings from being run, so order here matters. As standardKeymap
includes bindings matching most of our own customizations, those need to come first.
This PR doesn't fix the whole issue. We should either file a separate issue to track the implementation of the rich editor, or separate issues to track syntax highlighting and error annotations. |
Point taken. Removed the closing link. |
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.
Works very well, excellent job!
The only thing we should fix in addition to the CSS comment inline is add a short delay before we trigger trailing space indicator. Currently it's annoying to see the red color blinking as you type.
translate/src/modules/translationform/components/TranslationForm.css
Outdated
Show resolved
Hide resolved
translate/src/modules/translationform/components/TranslationForm.css
Outdated
Show resolved
Hide resolved
translate/src/modules/translationform/utils/editFieldExtensions.ts
Outdated
Show resolved
Hide resolved
translate/src/modules/translationform/utils/editFieldExtensions.ts
Outdated
Show resolved
Hide resolved
The default extension for this isn't configurable like that, so I removed it for now. |
This reverts commit 9a1ee55.
This reverts commit 9a1ee55.
This reverts commit 9a1ee55.
Fixes #2180
Fixes #2781
Replaces the translation editor
<textarea>
using CodeMirror, an editor we'll be able to extend to provide e.g. syntax and error highlighting as well as autocompletion.As CodeMirror isn't React, but does manage its internal state in a similar manner, matching that within the app requires some finessing. A new context
EditorResult
is added toEditorProvider
as an output; it's updated by the editor, but not read by it. The pre-existingEditorData
now no longer directly contains the value being edited, but does have references toEditFieldHandle
s, an interface that abstracts the get/set/focus operations for each field.