Skip to content

improvement: Wrap output for lists and dictionaries #2995

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.

Sign up for GitHub

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

Jump to bottom
Merged
mscolnick merged 4 commits into from
Dec 2, 2024
Merged

improvement: Wrap output for lists and dictionaries #2995

mscolnick merged 4 commits into from
Dec 2, 2024

Conversation

@wasimxyz
Copy link
Contributor

@wasimxyz wasimxyz commented Nov 28, 2024 •
edited
Loading

📝 Summary

Completes #2977.

🔍 Description of Changes

Add switch to wrap text for list and dictionary (JSON) outputs. Makes it easier to read long string outputs 🙂

Before:

Screenshot 2024-11-27 at 10 25 32 PM

After:

Screenshot 2024-11-27 at 10 25 16 PM

📋 Checklist

  • I have read the contributor guidelines.
  • For large changes, or changes that affect the public API: this change was discussed or approved through an issue, on Discord, or the community discussions (Please provide a link if applicable).
  • I have added tests for the changes made.
  • I have run the code and verified that it works as expected.

📜 Reviewers

@akshayka OR @mscolnick

@wasimxyz wasimxyz added the enhancement New feature or request label Nov 28, 2024
@wasimxyz wasimxyz requested a review from mscolnick November 28, 2024 06:31
@wasimxyz wasimxyz self-assigned this Nov 28, 2024
@vercel
Copy link

vercel bot commented Nov 28, 2024 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
marimo-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2024 8:43pm
marimo-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 29, 2024 8:43pm

@wasimxyz wasimxyz linked an issue Nov 28, 2024 that may be closed by this pull request
Wrap list and dictionary output #2977
Closed
akshayka
akshayka requested changes Nov 28, 2024
View reviewed changes
Copy link
Contributor

@akshayka akshayka left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. The UI is a bit obtrusive and distracting, and I can imagine it interfering with many people's existing apps.

Some thoughts.

Could the same functionality be done in a much less obtrusive way? Not using the switch component, but making it appear as if it were a native part of the JSON viewer. In fact, does the JSON component we use have this capability?

@wasimxyz
Copy link
Contributor Author

wasimxyz commented Nov 28, 2024

I'm open to different options! Switch was just the first that came to mind. And we're using texteainc/json-viewer, which doesn't seem to have wrapping capability.

@wasimxyz
Copy link
Contributor Author

wasimxyz commented Nov 28, 2024

Would a right click toggle or context menu option be preferable? A global keyboard shortcut would be even more hidden.

mscolnick
mscolnick previously approved these changes Nov 28, 2024
View reviewed changes
@akshayka
Copy link
Contributor

akshayka commented Nov 28, 2024

@wasimsandhu Thanks for the flexibility. Here is what I suggest.

  1. Remove the switch component.
  2. Wrap strings by default but also ...
  3. Set collapseStringsAfterLength to some reasonable number: https://viewer.textea.io/apis

In this way, when a long string is displayed, it won't eat up a ton of vertical space, but when the user expands it, they can still see the full text.

@wasimxyz
Copy link
Contributor Author

wasimxyz commented Nov 28, 2024 •
edited
Loading

That sounds great! Made the changes. The only problem is that collapseStringsAfterLength doesn't seem to work. 🙁 Currently investigating, until then I'll change this PR to a draft.

@wasimxyz wasimxyz marked this pull request as draft November 28, 2024 18:55
@wasimxyz
Copy link
Contributor Author

wasimxyz commented Nov 29, 2024

So collapseStringsAfterLength won't work with our custom leaf renderers. Got it working by implementing the collapsibility myself. Let me know if you'd like me to move it into its own file, I just left it in the same file.

Screenshot 2024-11-28 at 11 43 19 PM Screenshot 2024-11-28 at 11 43 29 PM

@wasimxyz wasimxyz marked this pull request as ready for review November 29, 2024 07:48
mscolnick
mscolnick reviewed Nov 29, 2024
View reviewed changes
frontend/src/components/editor/output/JsonOutput.tsx
{isCollapsed ? (
<span onClick={() => setIsCollapsed(false)}>
{props.text.slice(0, 500)}
{props.text.length > 500 && "..."}
Copy link
Contributor

@mscolnick mscolnick Nov 29, 2024

Choose a reason for hiding this comment

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

this changes the default case such that all text is hidden at 500. that is ok w/ me, but just point that out.

so if you have a lot of keys you want to expand, you'd have to click all of them (maybe not a common case)

Copy link
Contributor Author

@wasimxyz wasimxyz Nov 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

okay I'll add a comment there. as an aside, this is making me think that having a dedicated JSON viewer / editor plugin (outside the default one for outputs) would be nice to have...

edit: nevermind, looks like that's what mo.tree is for.

Copy link
Contributor

@akshayka akshayka Dec 2, 2024

Choose a reason for hiding this comment

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

Yep that's what mo.tree is for.

akshayka
akshayka approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@akshayka akshayka left a comment

Choose a reason for hiding this comment

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

Haven't played with it, but from the screenshot LGTM

@mscolnick mscolnick merged commit 9c47539 into main Dec 2, 2024
18 checks passed
@mscolnick mscolnick deleted the ws/wrap-json-output branch December 2, 2024 17:36
@ndc33
Copy link

ndc33 commented Apr 4, 2025

hi, this is not working for defaultdict

@mscolnick
Copy link
Contributor

mscolnick commented Apr 7, 2025

@ndc33 thanks for the callout, ive added it here: #4417

@ndc33
Copy link

ndc33 commented Apr 9, 2025

that was fast!
An additional issue with all output is the fixed font size
On a big monitor with large editor font, the output font stays tiny
I set CSS to scale marimo-json-output as a ratio of --marimo-code-editor-font-size

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mscolnick mscolnick mscolnick left review comments

@akshayka akshayka akshayka approved these changes

Assignees

@wasimxyz wasimxyz

Labels

enhancement New feature or request

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Wrap list and dictionary output

5 participants

@wasimxyz @akshayka @ndc33 @mscolnick