-
Notifications
You must be signed in to change notification settings - Fork 10
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
Support snippet/code param in Rzk playground #118
Conversation
9fe9a17
to
9e2a710
Compare
LGTM because I'm not a true typescripter. @aabounegm, what do you think? This PR provides initialization of the editor content from a url. |
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.
Excuse me for being so pedantic, but that might be my only opportunity :D
@@ -19,12 +19,20 @@ export default function Editor({ | |||
editorHeight: number | |||
}) { | |||
const [existsSelection, setExistsSelection] = useState(false) | |||
const params = new URLSearchParams(window.location.search); |
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.
Since this is a Next.js (not normal React.js) project, window
should not be used outside useEffect
since it is not available during SSR. I can see that the whole project is wrapped in next/dynamic
so it shouldn't be an issue in this specific situation, but that's not a good practice in general.
I wonder why you didn't just use the Vite template for React since we don't need any Next.js features.
var snippet = example | ||
if (params.has("snippet")) { | ||
snippet = params.get("snippet")!; | ||
} else if (params.has("code")) { | ||
snippet = params.get("code")!; | ||
} |
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.
First, always use let
instead of var
. Second, I would simplify it to avoid the intermediate variable as such:
var snippet = example | |
if (params.has("snippet")) { | |
snippet = params.get("snippet")!; | |
} else if (params.has("code")) { | |
snippet = params.get("code")!; | |
} | |
if (params.has("snippet")) { | |
setText(params.get("snippet")!); | |
} else if (params.has("code")) { | |
setText(params.get("code")!); | |
} else { | |
setText(example); | |
} |
preferable inside the same useEffect
in which params
should be defined.
Then you would also not need setText(snippet)
inside onCreateEditor
.
This might not be an optimal implementation. @deemp feel free to improve later.