Content-Length: 447448 | pFad | http://github.com/requestly/requestly/pull/2553

AA [ENGG-2788]refactor: Code editor component by Aarushsr12 · Pull Request #2553 · requestly/requestly · GitHub
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

[ENGG-2788]refactor: Code editor component #2553

Merged
merged 10 commits into from
Jan 18, 2025
Merged

[ENGG-2788]refactor: Code editor component #2553

merged 10 commits into from
Jan 18, 2025

Conversation

Aarushsr12
Copy link
Contributor

@Aarushsr12 Aarushsr12 commented Jan 13, 2025

PR focuses on:

  • Codemirror Cursor bug fixed [when typing fast cursor sets at the beginning of editor]
  • Refactored Editor component for API client only
  • Prettification in RequestBody & Response Body

hideCharacterCount = false,
handleChange = () => {},
toolbarOptions,
id = "",
Copy link
Contributor

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.

Copy link
Contributor Author

@Aarushsr12 Aarushsr12 Jan 14, 2025

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) {
Copy link
Contributor

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) => {
Copy link
Contributor

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) {
Copy link
Contributor

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
Copy link
Contributor

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?

Copy link
Contributor Author

@Aarushsr12 Aarushsr12 Jan 14, 2025

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 in editorToastAction.js , so to suppress that error from being reported in Editor.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 ? (
Copy link
Contributor

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.

Copy link
Contributor Author

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

@RuntimeTerror10 RuntimeTerror10 changed the title [ENGG-2788] chore: refactor editor component [ENGG-2788]refactor: Code editor component Jan 18, 2025
const toastOverlay = useMemo(() => allEditorToast[scriptId], [allEditorToast, scriptId]); // todo: rename
const [isCodePrettified, setIsCodePrettified] = useState(prettifyOnInit);
const isDefaultPrettificationDone = useRef(false);
const [editorContent, setEditorContent] = useState(value);
Copy link
Member

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.

Copy link
Contributor Author

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

Copy link
Contributor

@nsrCodes nsrCodes left a 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

@Aarushsr12 Aarushsr12 merged commit 758f2c4 into master Jan 18, 2025
2 checks passed
@Aarushsr12 Aarushsr12 deleted the ENGG-2788 branch January 18, 2025 15:23
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.

4 participants








ApplySandwichStrip

pFad - (p)hone/(F)rame/(a)nonymizer/(d)eclutterfier!      Saves Data!


--- a PPN by Garber Painting Akron. With Image Size Reduction included!

Fetched URL: http://github.com/requestly/requestly/pull/2553

Alternative Proxies:

Alternative Proxy

pFad Proxy

pFad v3 Proxy

pFad v4 Proxy