[SQL Lab] Add JSON modal when clicking on cells with JSON objects #7720
Conversation
etr2460
force-pushed
the
erik-ritter--sqllab-json-format
branch
from
bb0eb36
to
aa8e806
Compare
|
Oh, this is awesome! Quick question to you and @mistercrunch: can you change this to double click? I'm worried people won't be able to select the cell to copy the values if we use a single click for this. |
|
@betodealmeida I was thinking about selecting for copy pasting too... You can still do it as is, but i could see how it might be a bit finicky. What if we added a |
etr2460
force-pushed
the
erik-ritter--sqllab-json-format
branch
from
aa8e806
to
dc3c0f4
Compare
Codecov Report
@@ Coverage Diff @@
## master #7720 +/- ##
==========================================
- Coverage 65.77% 65.76% -0.01%
==========================================
Files 459 459
Lines 21911 21936 +25
Branches 2410 2414 +4
==========================================
+ Hits 14411 14426 +15
- Misses 7379 7389 +10
Partials 121 121
Continue to review full report at Codecov.
|
| import TooltipWrapper from '../TooltipWrapper'; | ||
|
|
||
| function getTextWidth(text, font = '12px Roboto') { | ||
| return getTextDimension({ text, style: { font } }).width; | ||
| } | ||
|
|
||
| function getJsonStringifiedObject(data) { | ||
| try { |
There was a problem hiding this comment.
In which scenario do you expect it to fail?
You can also use ../../../src/utils/safeStringify if the crash cases are circular json.
There was a problem hiding this comment.
It’ll fail if we don’t pass in valid json. Like “foo” or “{“. Let me know if there’s an easier way to determine if a string is valid json and resolves to an object/array
superset/assets/src/components/FilterableTable/FilterableTable.jsx
Outdated
Show resolved
Hide resolved
etr2460
force-pushed
the
erik-ritter--sqllab-json-format
branch
from
dc3c0f4
to
41bea2a
Compare
|
@etr2460 yeah, the copy button is great! LGTM, will defer to @mistercrunch since he requested changes. |
| @@ -165,6 +213,17 @@ export default class FilterableTable extends PureComponent { | |||
| this.setState({ sortBy, sortDirection }); | |||
| } | |||
|
|
|||
| addJsonModal(node, jsonObject, jsonString) { | |||
| return ( | |||
| <ModalTrigger | |||
There was a problem hiding this comment.
I'm not sure if perf matters here as I think we're always using react-virtualized in this context (only rendering the rows in the viewport), but I think this will build all of the modals and related <JSONTree />. @betodealmeida has been working on data preview perf recently and may have more intuition as to whether this matters.
An alternate approach would be to build / render the modal on only when clicked/used.
There was a problem hiding this comment.
Looking at the DOM and React devtools, it doesn't seem like anything gets rendered prior to clicking on the cell. The react structure has a ModalTrigger and an empty Modal element and that's it. I think we're probably fine here.
| import TooltipWrapper from '../TooltipWrapper'; | ||
|
|
||
| function getTextWidth(text, font = '12px Roboto') { | ||
| return getTextDimension({ text, style: { font } }).width; | ||
| } | ||
|
|
||
| function getJsonStringifiedObject(data) { |
There was a problem hiding this comment.
NIT (optional): naming safeJsonParse
There was a problem hiding this comment.
I'll make it safeJsonObjectParse since it doesn't parse all JSON, but only things that are objects.
etr2460
force-pushed
the
erik-ritter--sqllab-json-format
branch
from
41bea2a
to
35ef47b
Compare
|
@mistercrunch is this in good shape now? |
|
@etr2460 I just pulled your branch and tested it, this is awesome! <3 |
mistercrunch
added
🏷️ bot
CATEGORY
Choose one
SUMMARY
This was recommended in #7690 by @mistercrunch. Clicking on a JSON stringified object or array in SQL Lab now opens a modal with the formatted JSON object/array.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION
REVIEWERS
@mistercrunch @graceguo-supercat @betodealmeida @elibrumbaugh