-
Notifications
You must be signed in to change notification settings - Fork 323
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
feat(playground): Implement codemirror based template string editor #4943
feat(playground): Implement codemirror based template string editor #4943
Conversation
9540bfd
to
e505bd5
Compare
cf18e34
to
5990585
Compare
250e8d2
to
1499dd6
Compare
variant="quiet" | ||
aria-label={"Message content"} | ||
aria-label="Message content" | ||
templateLanguage={TemplateLanguages.Mustache} |
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.
Change this to TemplateLanguages.FString
to test that instead.
app/src/components/templateEditor/language/fStringTemplating/fStringTemplating.syntax.grammar
Outdated
Show resolved
Hide resolved
app/src/components/templateEditor/language/fStringTemplating/fStringTemplating.ts
Outdated
Show resolved
Hide resolved
* | ||
* If a variable is not found in this object, it will be left as is. | ||
*/ | ||
variables: Record<string, string | number | boolean>; |
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.
nit: your check below avoids this, but sometimes prefer writing records as Record<string, string | number | undefined> just because it's a little more honest and catches unset keys
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.
in my opinion we should just enable noUncheckedIndexedAccess
in our tsconfig if we are concerned about unset keys
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.
Added
...ponents/templateEditor/language/mustacheLikeTemplating/mustacheLikeTemplating.syntax.grammar
Outdated
Show resolved
Hide resolved
app/src/components/templateEditor/language/mustacheLikeTemplating/mustacheLikeTemplating.ts
Outdated
Show resolved
Hide resolved
app/src/components/templateEditor/language/mustacheLikeTemplating/mustacheLikeTemplating.ts
Outdated
Show resolved
Hide resolved
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.
cool stuff
19dde65
to
0d1c141
Compare
b109424
to
0284bda
Compare
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 so cool. Excited to wire it all up
app/src/components/templateEditor/language/__tests__/languageUtils.test.ts
Show resolved
Hide resolved
app/src/components/templateEditor/language/fStringTemplating/fStringTemplating.ts
Outdated
Show resolved
Hide resolved
}, [templateLanguage]); | ||
|
||
return ( | ||
<CodeMirror theme={codeMirrorTheme} extensions={extensions} {...props} /> |
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 see the ...props so I get that you are trying to keep this generic. Maybe you can expose under /code
a version that doesn't have things like line numbers since I think that's the primary use-case
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'm happy to tweak any defaults you want for TemplateEditor, including removing line numbers and setting a default height.
I tend to opt for exposing all props on components so that I don't have to add them later but I'm happy with leaving extra props opaque until they're actually needed to be drilled through
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.
…ncs into shared utils
0284bda
to
d1792fa
Compare
Alright I've fixed all of the parsing issues I could find for both langs, and implemented The final parse "issue" that I will leave unresolved for now is |
TODO: