-
Notifications
You must be signed in to change notification settings - Fork 0
Adjust rendering of tabs #21
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
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
03f9f89 to
d45d86f
Compare
d45d86f to
79f6150
Compare
5951cbd to
ed53f7d
Compare
iethree
left a comment
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 realize you're not a developer here, and I really appreciate how you're trying to push this work forward. My biggest piece of feedback is that this would have been much easier to deal with in separate PRs:
- add record count to tabs
- hide empty tabs
- restructure basicInfo data shape
- Move information to header
- Add json formatter
- Add SVGs for browsers + OSes
This isn't even so much because big chunks of code are hard to review, it's that if there are problems with any of them, they can be worked through independently, rather than blocking a bunch of easy changes behind some trickier ones
Absolutely, I realized that too late actually as I was originally "just" focusing on reordering tabs. As I was testing things out, one thing led to another and by pulling the thread, half of the sweater came. That's clearly something I'll worry more about if I ever do this again 😅 |
|
I went with separate commits to make the fixes easier to review in isolation |
| if (count == null) { | ||
| return null | ||
| } |
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.
moving this conditional in here lets us conditionally render these while making the parent component much more readable
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.
Indeed!
| <ScrollArea className="h-full w-full border overflow-auto bg-gray-100 rounded-md "> | ||
| <pre className="text-xs h-full flex p-4 whitespace-pre-wrap"> | ||
| {JSON.stringify(content, null, 2)} | ||
| </pre> | ||
| </ScrollArea> |
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.
the json formatting library was nice, but as i tested it more, it tended to crash the browser with larger data sets pretty consistently. Moving back to formatted raw text until we can find a more stable library
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.
Yeah that's too bad. I should have tested with large query results
|
Nice! |
Closes https://linear.app/metabase-inc/issue/DEVX-101/streamline-visualizer-to-focus-on-important-information
This one is giving a facelift to the debugger with the following changes:
Improvements
entityInfoto focus on checking out the entity detailsbugReportDetailssince it doesn't make much sense to bother formatting that furtherFixes
basicInfowrapper whenever uploading files directly to the debugger, which was making things messy.New dependencies
@svgr/webpackto facilitate imports, as well asvite-plugin-magical-svg(to prevent rendering issues in a testing context)react18-json-viewto render JSON much more nicely, enabling folding etc.Videos 🎥
Showing how it now looks and works
Running through the code changes