feat: Results table on Explore view #11854
Conversation
kgabryje
force-pushed
the
feature/explore-results
branch
from
cc0f6df
to
e774eaf
Compare
There was a problem hiding this comment.
great work, Kamil!
comments -
1. the caret and DATA mis alignment
2. View Samples is not showing on Table Viz
3. Data sorting order in the chart and table should be the same
4. let's make Data bold
5. set horizontal scroll bar to view all columns in VIEW RESULTS and VIEW SAMPLES
tested
Search bar ✅
#row retrieved ✅
tabs ✅
alignment ❌
Data showing on all charts ✅ except table viz, but it's an existing issue
showing the entire table/all columns ✅ except enormous amount of columns, existing issue
cosmetic ❌
resizing - legacy charts ✅
resizing - echarts ✅
|
Adding on to @junlincc 's numbering system:
This is looking AWESOME, btw, super excited about this. |
it did not work before, i agree this should not be a blocker |
currently, when there are tooooo many columns, clicking View Samples just crash the explore view. also happening in this PR. so 2,3,5 really are not new problems. we can merge this PR without them being addressed, but we should create separate PRs to fix those soon. no necessary by @kgabryje though. :) |
|
to confirm, this PR does offer horizontal scroll to show all columns(when it's not tooooo many like over 300 columns) |
@junlincc is that actually current behavior in master? Is there an issue tracking it? it looks pretty serious to me |
| @@ -0,0 +1,91 @@ | |||
| /** | |||
There was a problem hiding this comment.
this is a new file, can you please use TypeScript?
100%positive this is currently happening~ i agree this is serious, after this change, View Samples will be more accessible to the users,, which means crashes will more likely to happen as users poke around this new feature. #11908 thanks for reviewing btw! @etr2460 |
| @@ -49,22 +52,124 @@ const propTypes = { | |||
| triggerRender: PropTypes.bool, | |||
| }; | |||
|
|
|||
| export const EXPLORE_GUTTER_HEIGHT = 5; | |||
There was a problem hiding this comment.
I'd be tempted to see if we can live without these constants, and use gridUnit instead (which may require useTheme in some cases).
There was a problem hiding this comment.
I'm not sure about that. We use EXPLORE_GUTTER_MARGIN both in component logic and in a styled component. In case we wanted to change its value, we'd have to remember to modify 2 places instead of 1. Unless I passed that margin value as a prop to styled component? We'd benefit from using a theme variable, but I think we'd lose some readability. WDYT?
There was a problem hiding this comment.
Maybe there's a compromise, setting the EXPLORE_GUTTER_HEIGHT and EXPLORE_GUTTER_MARGIN constants to the number of gridUnits, and then using that as the multiplier - in this case they'd both be set to 1, I suppose. Which then raises the question of whether we need two consts or just use one unified gutter size const.
If this is all too much trouble or ruins the readability, maybe we can just change them both to 4 so they at least match the current world layout grid?
There was a problem hiding this comment.
I like this idea... but in this case 1px really does make a difference.
Here's a gutter height 4px:
And here's 5px:
Maybe the screenshots don't really show it, but in my opinion 5px looks much much better. Which raises a question, do we want do play around with 1.25 multiplier for gridUnit?
There was a problem hiding this comment.
I pushed a change with GUTTER_SIZE_FACTOR = 1.25. Let me know what you think 🙂
|
@junlincc Arrow centered and text bolded |
|
@junlincc I have a question related to searching in results: During my tests it stil crashes when I set big amount of data but as it was said - it is not introduced by this PR. @kgabryje Visual issue - while expanding data space there is a glitch. Maybe while expanding we can move also the bottom side? |
|
I haven't looked at the code but wanted to make sure that we don't fetch samples unless/until the tab is selected. I think that the "View Samples" use case is fairly uncommon. Also note that it only needs to be reprocess when the filters are altered, changing metrics and other query parameters don't require re-fetching/re-rendering. |
Currently both results and samples are fetched when user expands Data panel. They are refetched when query params change. I'll try to optimize that so that Samples are fetched only when needed |
|
@mistercrunch I noticed that on current master, we send a request for samples not only when chart data changes, but every time we open a modal. Should we consider refetching samples only when filters change as a separate feature? |
white background is much better, thanks for adding it! |
|
tested in Mozilla today.
remaining issues:
@etr2460 @rusackas any objections?
if this behavior is not new, we should address it separately. |
|
@kgabryje it's probably ok to only fetch samples when "View Samples" is first selected OR user hit run But logically I think it's only necessary to fetch the samples if the filtering has changed. |
|
@mistercrunch Shall we create a separate task for that? |
| } | ||
| `; | ||
|
|
||
| export const CopyToClipboardButton = ({ data }: { data: object }) => ( |
There was a problem hiding this comment.
I think we prefer Record<string, any> to object for typescript
|
|
||
| export const CopyToClipboardButton = ({ data }: { data: object }) => ( | ||
| <CopyToClipboard | ||
| text={data ? prepareCopyToClipboardTabularData(data) : ''} |
There was a problem hiding this comment.
if data is always an object, then this check shouldn't be needed
There was a problem hiding this comment.
It actually can be undefined, I changed data: object to data?: Record<string, any>
| ); | ||
| }; | ||
|
|
||
| export const RowCount = ({ data }: { data: object[] }) => ( |
There was a problem hiding this comment.
same comment here about the typing of object
| <RowCountLabel rowcount={data?.length ?? 0} suffix={t('rows retrieved')} /> | ||
| ); | ||
|
|
||
| export const useFilteredTableData = (filterText: string, data?: object[]) => |
There was a problem hiding this comment.
same comment here about the typing of object
| ); | ||
| }, [data, filterText]); | ||
|
|
||
| export const useTableColumns = (data?: object[]) => |
There was a problem hiding this comment.
same comment here about the typing of object (i'm going to stop making these, but please clean up the rest, thanks!)
There was a problem hiding this comment.
Got it! Thanks for pointing that out. Fixed here and in other types
| return []; | ||
| } | ||
| const formattedData = applyFormattingToTabularData(data); | ||
| return formattedData.filter((row: object) => |
There was a problem hiding this comment.
same comment here about the typing of object
merge to test feat: Results table on Explore view apache#11854
merge to test feat: Results table on Explore view apache#11854
merge to test feat: Results table on Explore view apache#11854
merge to test feat: Results table on Explore view apache#11854
mistercrunch
added
🏷️ bot
SUMMARY
This PR implements feature described in apache-superset/superset-roadmap#61.
The goal was to move
View resultsandView samplestables fromDisplayQueryButtonto main Explore view. Tables are placed in tabs, in a collapsible section.The whole chart/table area is resizable, with tables section initially collapsed and with minimal height to show as much of the chart as possible. After "uncollapsing" it automatically resizes to 40% if container height, leaving 60% for the chart. The heights can be further adjusted with split handle. After collapsing, it shrinks back to its original height.
The implemented UI is a bit different compared to the one linked in the issue - changes were discussed with @junlincc and @rusackas.
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION
CC: @junlincc @rusackas @villebro
@adam-stasiak @agatapst Could you please help with manual testing?