-
Notifications
You must be signed in to change notification settings - Fork 50.2k
[playground] bug fixes & UX improvements #34499
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
Changes from all commits
1e18d65
88ccbb4
09e8ef7
6faa9f1
0788958
c8fb918
46ffa19
5e5a84d
a7bb236
16561a4
10d938c
ef87362
0e0fcb0
c0769ce
a128cac
84f2e54
2004c12
f527d55
d84a587
b201a1d
bdde8ec
87339bb
8263b5e
201301e
8a7d036
59f54a2
d158da1
030099e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -9,7 +9,7 @@ import MonacoEditor, {loader, type Monaco} from '@monaco-editor/react'; | |
| import {PluginOptions} from 'babel-plugin-react-compiler'; | ||
| import type {editor} from 'monaco-editor'; | ||
| import * as monaco from 'monaco-editor'; | ||
| import React, {useState} from 'react'; | ||
| import React, {useState, useRef, useEffect} from 'react'; | ||
| import {Resizable} from 're-resizable'; | ||
| import {useStore, useStoreDispatch} from '../StoreContext'; | ||
| import {monacoOptions} from './monacoOptions'; | ||
|
|
@@ -28,10 +28,25 @@ export default function ConfigEditor({ | |
| }): React.ReactElement { | ||
| const [isExpanded, setIsExpanded] = useState(false); | ||
|
|
||
| return isExpanded ? ( | ||
| <ExpandedEditor onToggle={setIsExpanded} appliedOptions={appliedOptions} /> | ||
| ) : ( | ||
| <CollapsedEditor onToggle={setIsExpanded} /> | ||
| return ( | ||
| // TODO: Use <Activity> when it is compatible with Monaco: https://github.com/suren-atoyan/monaco-react/issues/753 | ||
| <> | ||
| <div | ||
| style={{ | ||
| display: isExpanded ? 'block' : 'none', | ||
EugeneChoi4 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }}> | ||
| <ExpandedEditor | ||
| onToggle={setIsExpanded} | ||
| appliedOptions={appliedOptions} | ||
| /> | ||
| </div> | ||
| <div | ||
| style={{ | ||
| display: !isExpanded ? 'block' : 'none', | ||
| }}> | ||
| <CollapsedEditor onToggle={setIsExpanded} /> | ||
| </div> | ||
| </> | ||
| ); | ||
| } | ||
|
|
||
|
|
@@ -44,16 +59,25 @@ function ExpandedEditor({ | |
| }): React.ReactElement { | ||
| const store = useStore(); | ||
| const dispatchStore = useStoreDispatch(); | ||
| const debounceTimerRef = useRef<NodeJS.Timeout | null>(null); | ||
|
|
||
| const handleChange: (value: string | undefined) => void = value => { | ||
| const handleChange: (value: string | undefined) => void = ( | ||
| value: string | undefined, | ||
| ) => { | ||
| if (value === undefined) return; | ||
|
|
||
| dispatchStore({ | ||
| type: 'updateConfig', | ||
| payload: { | ||
| config: value, | ||
| }, | ||
| }); | ||
| if (debounceTimerRef.current) { | ||
| clearTimeout(debounceTimerRef.current); | ||
| } | ||
|
|
||
| debounceTimerRef.current = setTimeout(() => { | ||
| dispatchStore({ | ||
| type: 'updateConfig', | ||
| payload: { | ||
| config: value, | ||
| }, | ||
| }); | ||
| }, 500); // 500ms debounce delay | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we use useDeferredValue() instead of manually debouncing?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
It seemed like these updates were still happening too quickly while typing in the config editor so I tried adding a manual debouncing mechanism with a longer timeout window.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @josephsavona we're adding debouncing to reduce thrash in the output panel from JS parse errors |
||
| }; | ||
|
|
||
| const handleMount: ( | ||
|
|
@@ -77,12 +101,6 @@ function ExpandedEditor({ | |
| allowSyntheticDefaultImports: true, | ||
| jsx: monaco.languages.typescript.JsxEmit.React, | ||
| }); | ||
|
|
||
| const uri = monaco.Uri.parse(`file:///config.ts`); | ||
| const model = monaco.editor.getModel(uri); | ||
| if (model) { | ||
| model.updateOptions({tabSize: 2}); | ||
| } | ||
| }; | ||
|
|
||
| const formattedAppliedOptions = appliedOptions | ||
|
|
@@ -126,6 +144,7 @@ function ExpandedEditor({ | |
| value={store.config} | ||
| onMount={handleMount} | ||
| onChange={handleChange} | ||
| loading={''} | ||
| options={{ | ||
| ...monacoOptions, | ||
| lineNumbers: 'off', | ||
|
|
@@ -139,7 +158,6 @@ function ExpandedEditor({ | |
| /> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="flex-1 flex flex-col m-2"> | ||
| <div className="pb-2"> | ||
| <h2 className="inline-block text-blue-50 py-1.5 px-1.5 xs:px-3 sm:px-4 text-sm"> | ||
|
|
@@ -151,6 +169,7 @@ function ExpandedEditor({ | |
| path={'applied-config.js'} | ||
| language={'javascript'} | ||
| value={formattedAppliedOptions} | ||
| loading={''} | ||
| options={{ | ||
| ...monacoOptions, | ||
| lineNumbers: 'off', | ||
|
|
||
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.
Seems like this has gone back and forth a couple times. As a follow up there could be a test that starts the app both with internals showing and not showing.
Also/alternatively adding some testID to the tabs would allow for better selecting. like
'[data-testid="config-tab"] .monaco-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.
Yup, I'm actually cleaning up/adding more tests in another PR right now so this will be addressed there.