-
Notifications
You must be signed in to change notification settings - Fork 0
feat(Codebytes): add simple monaco editor disc 353 #16
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
Conversation
packages/codebytes/src/editor.tsx
Outdated
| ${({ hasError, theme }) => ` | ||
| color: ${hasError ? theme.colors.orange : theme.colors.white}; | ||
| background-color: ${theme.colors['gray-900']}; | ||
| background-color: ${colors['gray-900']}; |
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 forget which we prefer theme.colors or importing colors directly but we should go with one instead of both :)
(we use theme.colors in the row above)
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, this is a really interesting one! The theme.colors['gray-900'], is different color than the ${colors['gray-900']}. We need the color that comes up in ${colors['gray-900']} but, yeah, colors is definitely deprecated. I'll shoot a message to @dreamwasp and see what the best approach is for a deprecated color?
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 thats not great. definitely should loop in gamut team for this
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.
spoke to cass, the new color is ${theme.colors['navy-900']}, so we're covered!
packages/codebytes/package.json
Outdated
| "react-monaco-editor": "0.34.0", | ||
| "@monaco-editor/react": "4.3.1", | ||
| "react-resize-observer": "1.1.1", | ||
| "monaco-editor": "0.20.0" |
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 we are working with the latest version of @monaco-editor/react it may be worth updating the monaco-editor version as well
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.
awesome, thank you for the heads up!
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.
@aionate0812 The amazing thing is that I wasn't getting syntax highlighting before, but with the package updates, I am 🎉
| onMount={editorWillMount} | ||
| onChange={onChange} | ||
| options={{ minimap: { enabled: false } }} | ||
| theme="vs-dark" |
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 resolved some of the flashing issues we were seeing earlier. It's using monaco's built in dark theme initially, before we add a custom dark theme. In line with how the component currently works.
| import { dark } from './theme'; | ||
| import { Monaco } from './types'; | ||
|
|
||
| const ReactMonacoEditor = loadable(() => import('@monaco-editor/react')); |
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.
not sure how this may play with Next.js as it uses dynamic imports instead
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.
Best solution here might be to keep this as a normal import, and have the parent component decide to use dynamic imports (next.js) or loadable (monolith) when calling the CodeByteEditor. Updating to this approach for now.
| @@ -0,0 +1 @@ | |||
| CONTAINER_API_BASE=propeller.cc-le-cf.com | |||
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.
why are we using the prod endpoint in local? and can you set up the staging and prod ones too?
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.
@borisonr jake mentioned that since this is open source, then we want users to only be aware of our prod endpoint, not our staging one.
if we really do want to set up a staging endpoint here, which we would only need once we build a gamut-like storybook experience on a server, we'd probably need to do this in circleci or github actions. else, it's not essential.
for the current purposes, i think this covers what we need! cc'ing @jakemhiller to get some more input, too!
(eventually, the parent/container calling codebytes will pass in a staging/prod endpoint of its own)
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.
thats fine! but maybe we dont call this file local then? just have one env file
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.
totally reasonable!
borisonr
left a comment
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.
we're getting so close!!
📬Published Alpha Packages:@codecademy/codebytes@0.2.1-alpha.4f65b1.0 |
* origin/main: chore(release): publish chore: bumped codebytes to next version (#25) Revert "add yarn build task for codebytes (#23)" (#24) add yarn build task for codebytes (#23) chore(release): publish feat(Codebytes): Refactor tracking in Codebytes (#19) chore(release): publish feat(Codebytes): move language selection component disc 354 (#17) chore(release): publish feat(Codebytes): add simple monaco editor disc 353 (#16) chore(release): publish feat(Codebytes): add editor and drawers disc 351 (#14) chore(release): publish feat(Tracking): add search id param to userclick data chore(release): publish feat(Codebytes): move codebytes parent disc 351 (#11) chore(release): publish fix(tracking): remove navigator.sendBeacon .bind and add try/catch
Overview
Feature: Migrate SimpleMonacoEditor to Codebytes in Client-Modules
PR Checklist
This pull request is part of a series of pull requests to migrate the codebytes UI to client modules. The component structure thus far is:
CodebyteEditor > Editor > Drawers > SimpleMonacoEditorSome notes:
PR Checklist
To see the component locally:
http://localhost:6006/