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

Multi-line input for DevTools Console #454

Merged
merged 16 commits into from
Jan 16, 2018

Conversation

xyc
Copy link
Contributor

@xyc xyc commented Jan 14, 2018

What kind of change does this PR introduce?
Multi-line input support for DevTools Console like VSCode console / Chrome console. It's implemented using Monaco (similar to VSCode implementation).

Also fixes issues where

  • command cursor is not cleared on evaluating the command (for example if you press up arrow and edit the content, then press enter, the next up arrow should still bring you the immediate previous command)
  • in firefox height is not correct when you have many messages.

What is the current behavior?
Console input cannot have multiple lines.

What is the new behavior?
Console input has multi-line support. Shift + Enter goes to the next line. Enter evaluates the command. Up and Down arrow works as expected. You can paste multiple lines into the prompt as well.
multi-line input

Checklist:

  • Documentation N/A
  • Tests N/A
  • Ready to be merged
  • Added myself to contributors table

Copy link
Member

@CompuIves CompuIves left a comment

Choose a reason for hiding this comment

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

Wow, this is super impressive. Thank you so much for this addition and the time you've put into it! I love the details and how polished it is + works. I have one remaining question, other than we can merge it in ASAP 😄

type Props = {
evaluateConsole: (command: string) => void,
const monacoOptions = {
// language: 'javascript',
Copy link
Member

Choose a reason for hiding this comment

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

Why did you comment this line?

Copy link
Contributor Author

@xyc xyc Jan 15, 2018

Choose a reason for hiding this comment

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

Thanks @CompuIves ! Pleasure to help.

Do you want syntax highlighting and autosuggestion as well 😃 ? (if we uncomment the line)
syntax-highlight

But it looks that in Monaco all editors share only one theme, and if it's not specified it will be the default theme. (microsoft/monaco-editor#338) In code mirror mode the theme is not defined.

We would need to detect if we are in CodeMirror mode from the user preference. We would need to define theme like how Monaco.js did if we are in code mirror mode, or fall back to single line input for simplicity.

Edit: Alternative approach is to extract defineTheme logic from Monaco.js into MonacoReactComponent/a separate file and have a guard that prevents the theme from being defined twice. That way even in code mirror mode we shall still have the theme available. (defining theme on input component mounting will sometimes result in text re-colorized briefly in other Monaco editors so it's best that theme is only defined once).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The above changes (extracted to a separate file): xyc@8b4f76d. Please let me know what you think @CompuIves

Copy link
Member

@CompuIves CompuIves Jan 15, 2018

Choose a reason for hiding this comment

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

Edit: Alternative approach is to extract defineTheme logic from Monaco.js into MonacoReactComponent/a separate file and have a guard that prevents the theme from being defined twice. That way even in code mirror mode we shall still have the theme available. (defining theme on input component mounting will sometimes result in text re-colorized briefly in other Monaco editors so it's best that theme is only defined once).

I like this approach, sounds good!

Syntax Highlighting and autosuggest are reaaally cool. Makes it a lot easier to work with the console!

@xyc
Copy link
Contributor Author

xyc commented Jan 16, 2018

Cool I've merged in the syntax highlighting changes and merged with upstream/master. Should be ready to merge now. Thanks for taking your time to review this!

@CompuIves
Copy link
Member

Maaan you deserve all the thanks! I love this feature, and the amount of polish you've added is impressive. Thanks for your contribution!

@CompuIves CompuIves merged commit 3847663 into codesandbox:master Jan 16, 2018
@xyc
Copy link
Contributor Author

xyc commented Jan 17, 2018

Thanks for the opportunity to contribute! Love what you've been doing with CodeSandbox!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants