feat: SIP-34 card/grid views for dashboards and charts #10526
Conversation
|
My first impression visually (more of a note for our designers) is that we could have 4 columns in the grid, maybe even 5. Super stoked to see this! |
|
@mistercrunch the screenshots were taken on my retina laptop screen simulating
Initially design specified behavior where the cards would also scale with the window up until a point but we ended up deferring that implementation until we had a better framework for responsiveness in superset. |
Codecov Report
@@ Coverage Diff @@
## master #10526 +/- ##
==========================================
- Coverage 63.60% 59.44% -4.16%
==========================================
Files 763 358 -405
Lines 36120 22918 -13202
Branches 3408 0 -3408
==========================================
- Hits 22973 13624 -9349
+ Misses 13034 9294 -3740
+ Partials 113 0 -113
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
|
The rails look very wide, how do other tools deal with this. [off to pinterest.com ...] Looks like the rails between cards is constant, but the leftmost and rightmost rails are dynamic and the cards that fit in are centered. |
| } | ||
|
|
||
| const CardContainer = styled.div` | ||
| justify-content: space-between; |
There was a problem hiding this comment.
This looks fine when there are 3+ columns, but with 2 columns the cards look like they're pushed to the edges of the container:
Using justify-content: space-around would space them more evenly:
There was a problem hiding this comment.
I ended up ditching the flexbox implementation in favor of CSS grid. I really didn't like how this was sometimes happening on the last row
|
|
||
| .cover-footer { | ||
| transform: translateY(36px); | ||
| transition: 0.2s ease-out; |
There was a problem hiding this comment.
theme.transitionTiming is 0.3 if you wanna base this on that var.
| const CardCoverImg = styled.img` | ||
| display: block; | ||
| object-fit: cover; | ||
| width: 459px; |
There was a problem hiding this comment.
Could just be 100% width, and auto height if the card has its own width set, I think?
| th { | ||
| background: white; |
|
|
||
| .actions { | ||
| white-space: nowrap; | ||
| font-size: 24px; |
There was a problem hiding this comment.
Hmmmm... the theme has
xl: 21,
xxl: 28
... I wonder if we should use one of those, and possibly adjust the value if needed. The current theme values are based on the LESS variables file, and might need to be updated to more closely match SIP-34 specs.
There was a problem hiding this comment.
actually, I don't think this is even necessary anymore. This is left over from the days of the font-awesome icons.
| className={cx('table-cell', { | ||
| 'table-cell-loader': loading, | ||
| [cell.column.size || '']: cell.column.size, | ||
| <TableContainer> |
There was a problem hiding this comment.
If Table is a styled table is there any need for this TableContainer to exist? Seems like those styles could just be part of the Table styles.
| {headerGroups.map(headerGroup => ( | ||
| <tr {...headerGroup.getHeaderGroupProps()}> | ||
| {headerGroup.headers.map(column => { | ||
| let sortIcon = <Icon name="sort" />; |
There was a problem hiding this comment.
Not sure if this is easier or harder to parse ¯\_(ツ)_/¯ :
<Icon name={column.isSorted ? (column.isSortedDesc ? "sort-desc" : "sort-desc") : "sort" } />
Also, you could just set the value of sortIconName and then use it inline later in rendering, i.e. {column.canSort && <Icon name={sortIconName} />
There was a problem hiding this comment.
These are just indentation changes, so sorry if I'm off target ;)
There was a problem hiding this comment.
i'm not sure we're allowed nested ternaries per our eslint rules. I've already caused a bug trying to "simplify" this logic once, I'd like to avoid touching this logic again unless necessary.
| `; | ||
|
|
||
| const CardWrapper = styled.div` | ||
| display: inline-block; |
There was a problem hiding this comment.
Not sure this is needed, if the parent is a grid?
Actually, I wonder if CardWrapper is needed at all, if the key could be passed into the renderCard method.
There was a problem hiding this comment.
I think it's good to have a wrapper, this ensures the key is always present. I don't think the implementation of renderCard should have to know that it is responsible for adding the key prop to the resulting JSX. It's something I often forget. This also provides a wrapper div to implement additional functionality, eg bulk select. I'll remove the unnecessary styling though.
| @@ -21,20 +21,28 @@ import { t } from '@superset-ui/translation'; | |||
| import PropTypes from 'prop-types'; | |||
| import React from 'react'; | |||
| import rison from 'rison'; | |||
| import { Label } from 'react-bootstrap'; | |||
There was a problem hiding this comment.
Could this be the Label from src/components/Label?
There was a problem hiding this comment.
Did that PR already get merged? I'll rebase and use that.
Co-authored-by: Evan Rusackas <evan@preset.io>
|
Impacts #8976 |
mistercrunch
added
🏷️ bot
SUMMARY
BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
After:
TEST PLAN
ADDITIONAL INFORMATION
ENABLE_REACT_CRUD_VIEWSflag)