-
Notifications
You must be signed in to change notification settings - Fork 14k
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
fix(sql lab): NULL styling in grid cell #17385
Conversation
Codecov Report
@@ Coverage Diff @@
## master #17385 +/- ##
==========================================
- Coverage 77.07% 76.94% -0.13%
==========================================
Files 1036 1038 +2
Lines 55756 56009 +253
Branches 7630 7735 +105
==========================================
+ Hits 42972 43098 +126
- Misses 12529 12654 +125
- Partials 255 257 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
</i> | ||
) : ( | ||
this.getCellContent({ cellData, columnKey }) | ||
); |
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.
do we still need this if the table has more than 50 columns? (i.e., renders a grid?)
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.
Maybe we could just move the logic to getCellContent
?
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.
Oops, good catch! Added it back in this commit
.
My initial attempt was to add this logic to getCellContent
, but that function needs to get just the text content of each cell, it doesn't like components. It uses the text to get the width of the columns. I tried working around the function to get it to accept elements too but I couldn't find a way, since it needs to output just a string of what's inside the cell for sizing purposes.
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.
Can you confirm that it also works with more than 50 columns?
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've updated the description with screenshots for table and grid
/testenv up |
@eschutho Ephemeral environment spinning up at http://52.32.150.39:8080. Credentials are |
Ephemeral environment shutdown and build artifacts deleted. |
* Fix NULL styling in gridCell * removed unnecessary imports * Added null styling back to renderGridCell * Cleaned up a little code as per Elizabeth * Found another little cleanup spot
SUMMARY
NULL
grid cells in SQL Lab's result set were no longer returning their muted styling (italicized and muted gray). This was fixed by applying theNULL
styles through therenderTableCell
function instead ofrenderGridCell
.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
BEFORE
AFTER
TESTING INSTRUCTIONS
NULL
valueADDITIONAL INFORMATION