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

feat(playground): Implement codemirror based template string editor #4943

Merged

Conversation

cephalization
Copy link
Contributor

@cephalization cephalization commented Oct 10, 2024

TODO:

  • Implement tests
    • Need to figure out rollup plugins in jest or switch to vitest
  • Implement mustache like template language
    • Should be a quick-ish port just copy-pasting the fstring grammar and adding double brackets
  • Implement variable replacement util that utilizes parsed tree

@cephalization cephalization force-pushed the cephalization/playground-codemirror-syntax-highlighting branch from 9540bfd to e505bd5 Compare October 11, 2024 13:53
@cephalization cephalization force-pushed the cephalization/playground-codemirror-syntax-highlighting branch from 250e8d2 to 1499dd6 Compare October 11, 2024 18:03
@cephalization
Copy link
Contributor Author

This is the last known mustache syntax bug I can find.

Triple backticks selects the left two most braces and the inner most two right braces and includes one brace as input. Desired behavior is probably to escape this entirely.

image

variant="quiet"
aria-label={"Message content"}
aria-label="Message content"
templateLanguage={TemplateLanguages.Mustache}
Copy link
Contributor Author

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.

@cephalization cephalization marked this pull request as ready for review October 11, 2024 19:38
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Oct 11, 2024
*
* If a variable is not found in this object, it will be left as is.
*/
variables: Record<string, string | number | boolean>;
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added

Copy link
Contributor

@Parker-Stafford Parker-Stafford left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool stuff

@cephalization cephalization force-pushed the cephalization/playground-codemirror-syntax-highlighting branch from b109424 to 0284bda Compare October 11, 2024 20:34
@mikeldking
Copy link
Contributor

I think there's some very minor syntax issues e.g. raw {{}}

Screenshot 2024-10-11 at 5 59 36 PM

@mikeldking
Copy link
Contributor

Light mode looks "unhighlighted" for some reason - probably need a different theme?
Screenshot 2024-10-11 at 6 01 08 PM

Copy link
Contributor

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

}, [templateLanguage]);

return (
<CodeMirror theme={codeMirrorTheme} extensions={extensions} {...props} />
Copy link
Contributor

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

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

No more gutter, line numbers, or line highlight by default

@cephalization
Copy link
Contributor Author

Light mode looks "unhighlighted" for some reason - probably need a different theme?
Screenshot 2024-10-11 at 6 01 08 PM

Correct. Highlighting is determined by setting a semantic tag type and then the theme is meant to handle the actual coloring. We have no theme set on light mode, similar to the other CodeMirror editor. I'm happy to choose one but feel free to pick one as well and I'll add it

@cephalization
Copy link
Contributor Author

I think there's some very minor syntax issues e.g. raw {{}}

Screenshot 2024-10-11 at 5 59 36 PM

Mustache surprisingly has more parsing quirks than f-string. I'll spend a bit more time refining it but I'll have to be careful not to rabbit hole on it, I keep nerd sniping myself into optimization and tweaking on this

@cephalization cephalization force-pushed the cephalization/playground-codemirror-syntax-highlighting branch from 0284bda to d1792fa Compare October 12, 2024 21:05
@cephalization
Copy link
Contributor Author

Alright I've fixed all of the parsing issues I could find for both langs, and implemented \ escaping and fstring {{escape}} as well as mustache {{{ escape }}}.

The final parse "issue" that I will leave unresolved for now is {test {value}} The highlighter stops at the second to last brace, and the emitted variable name is test {value

@cephalization
Copy link
Contributor Author

image

Light mode theme "github"

@cephalization cephalization merged commit e20716f into playground Oct 14, 2024
7 checks passed
@cephalization cephalization deleted the cephalization/playground-codemirror-syntax-highlighting branch October 14, 2024 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants