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

Xterm.js Option #1317

Merged
merged 48 commits into from May 29, 2023
Merged

Xterm.js Option #1317

merged 48 commits into from May 29, 2023

Conversation

JeffersGlass
Copy link
Member

@JeffersGlass JeffersGlass commented Mar 27, 2023

Adds a new options xterm to <py-config>, which defaults to false. If xterm is true, the xterm.js package is dynamically loaded and used as py-terminal, in place of the default <pre>-tag-with-formatting. This is ~400kb of additional transfer, so xterm.js is only fetched if this option is enabled.

This PR is ready for review

image

This PR is a Work in Progress. To-do list is:

  • Get Xterm working as py-terminal
  • Enable screen reader mode by default for accessibility
  • Rebase following Syncify PR Merge
  • Investigate better typing for modules loaded from URL
  • Investigate xterm's auto-sizing function to make sure there are no hidden issues rendering on page load Auto-fit addon is pretty buggy, not going to include it
  • Think about setting some options in os.environ to make common Python terminal utilities work "out of the box"
    • Not sure if these options should be set by default, or documented as recommended things for users to do themselves. Leaning toward the latter.
    • Termcolor
    • Colorama (works with no additional config)
    • Rich - Need to set rich._console = RichConsole(color_system="...")
    • curses not built/bundled with Pyodide
  • New Tests
  • Add parameter validation once Improve validate() function for plugin options #1323 is merged
  • Documentation
  • Changelog

@JeffersGlass JeffersGlass added the tag: rendering Related to rendering of output into the DOM label Mar 27, 2023
@JeffersGlass
Copy link
Member Author

I noted this over there was well, but I want to include the functionality from #1323 in this, so prioritizing that PR for the moment.

@JeffersGlass
Copy link
Member Author

test_simple_clock failed, but certainly passes locally... very odd.

@JeffersGlass
Copy link
Member Author

JeffersGlass commented May 2, 2023

Thank you so much for all the comments/notes @WebReflection ! I've addressed most of them, but the ones around the general approach using the CDN as well as guarding against what happens when a node gets moved around I've not yet addressed, so we can continue conversation on them.

@WebReflection
Copy link
Contributor

@JeffersGlass no problem and, if needed, I can help resolving pyscriptjs/src/plugins/pyterminal.ts as that's "on me" but if you don't find particular issues go ahead 👍

@WebReflection
Copy link
Contributor

@JeffersGlass it looks like there are too many unrelated files in here ... are you tackling the rebase or do you need any help with that?

@JeffersGlass
Copy link
Member Author

@WebReflection Yeah it looks like I screwed up the merge from main - just remerged (including the PR's from earlier today), it's back down to a much more reasonable 7 files changed. Haven't looked at your other responses yet, but I appreciate them!

@WebReflection
Copy link
Contributor

@JeffersGlass few comments / nits you can freely ignore if you think my opinion is not so relevant, otherwise this LGTM and thanks for working on it, it looks awesome!

@JeffersGlass JeffersGlass merged commit 932756c into pyscript:main May 29, 2023
6 checks passed
@JeffersGlass
Copy link
Member Author

Thanks all! And especially thanks to you @WebReflection for the many rounds of revision and review, and really helping shape the API and naming of this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: ready PR that is ready for review tag: rendering Related to rendering of output into the DOM
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants