-
Notifications
You must be signed in to change notification settings - Fork 223
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
[ENGG-2788]refactor: Code editor component #2553
Conversation
hideCharacterCount = false, | ||
handleChange = () => {}, | ||
toolbarOptions, | ||
id = "", |
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.
Hey, just wanted to understand the repercussions of this. If I have two editor instances on the same page, and I didn't provide id
to any of them. Would that mean two dom nodes with same id?
Side note: I don't see this id being used anywhere.
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.
id
prop contains the script id passed from custom script row
- it is being used in EditorToastContainer to give warning in insert script rule
EditorToastContainer
component does not effect editor v2, we can keep this or either remove it- we can also remove this from here and define it in custom script row component
}; | ||
|
||
const updateContent = useCallback((prettifiedCode: string) => { | ||
if (editorRef.current?.view) { |
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.
Code nitpick since it makes it a little easier:
() => {
const view = editorRef.current?.view;
if(!view) {
return;
}
// Use view normally here
}
} | ||
}; | ||
|
||
const updateContent = useCallback((prettifiedCode: string) => { |
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 signature makes it look like this only updates prettified code but the code looks like it can update the entirety of content.
if (isEditorInitialized && !isDefaultPrettificationDone.current) { | ||
applyPrettification(); | ||
isDefaultPrettificationDone.current = true; | ||
} else if (!prettifyOnInit) { |
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.
Can't we reach this branch if editor is not initialized? And then we might attempt to prettify even in that scenario?
|
||
const handleEditorClose = useCallback( | ||
(id: string) => { | ||
// @ts-expect-error |
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.
Can you please add some context here? Why expect error?
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.
// @ts-expect-error
dispatch(globalActions.removeToastForEditor({ id }));
removeToastForEditor
reducer function is ineditorToastAction.js
, so to suppress that error from being reported inEditor.tsx
file- To remove this need to change editorToastAction to
editorToastAction.ts
- This is also part of EditorToast, which effects only script rule
[] | ||
); | ||
|
||
return isFullScreen ? ( |
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.
Please create either smaller components in the same file, or local variables in this function. And then use them in conditional branches.
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.
Defined local variables in function
const toastOverlay = useMemo(() => allEditorToast[scriptId], [allEditorToast, scriptId]); // todo: rename | ||
const [isCodePrettified, setIsCodePrettified] = useState(prettifyOnInit); | ||
const isDefaultPrettificationDone = useRef(false); | ||
const [editorContent, setEditorContent] = useState(value); |
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 now creates 2 source of truths for editor component. some effects use value
prop and editorContent
. we should have a single source of truth.
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 is fixed, removed the second state, considering only the value to be consistent source of truth
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.
communicated suggestions over slack, looks good for now
PR focuses on: