feat(component-library): table grid #3088
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
| const mappedColDefs = useMemo(() => { | ||
| return colDefs.map((colDef) => { | ||
| return { | ||
| ...colDef, | ||
| // the types in ag-grid are `any` for the cellRenderers which is throwing an error here | ||
| // eslint-disable-next-line @typescript-eslint/no-unsafe-assignment | ||
| cellRenderer: isLoading | ||
| ? (colDef.loadingCellRenderer ?? SkeletonCellRenderer) | ||
| : colDef.cellRenderer, | ||
| }; | ||
| }); | ||
| }, [colDefs, isLoading]); |
There was a problem hiding this comment.
IMO we should try to avoid mapping colDefs as much as we possibly can, since it implicitly means we've added an abstraction layer on top of AG grid, and in my experience this quickly becomes very messy.
For this specific use case is there any chance we can leverage the cellRendererSelector + defaultColDef instead?
Also - is it really correct that we should swap out the renderers of all cells instead of showing a loading overlay?
There was a problem hiding this comment.
I wanted to have a nicer loading state rather than disabling the whole thing - especially if the grid is full screen height. DIdn't know about about cellRendererSelector I'll try it out.
There was a problem hiding this comment.
Later edit: actually cellRendererSelector is on a per cell basis so we would still be doing the same mapping over colDefs, and also it might not work as its a function that uses the row data, so we might need to recreate this function that returns different renderers based on the loading state.
I propose we leave it like this for now and if it causes issue then we can remove it favor of the default loading overlay. It's very surprising it's so difficult to have a loading skeleton instead of the default loading overlay - which is a much better UX.
There was a problem hiding this comment.
There's also a loadingOverlayComponent param we can pass to the grid btw, but maybe you've already looked at that.
| const PriceCellRenderer = ({ value }: { value: number }) => ( | ||
| <span style={{ height: "100%", display: "flex", alignItems: "center" }}> | ||
| {`$${value.toLocaleString(undefined, { | ||
| minimumFractionDigits: 2, | ||
| maximumFractionDigits: 2, | ||
| })}`} | ||
| </span> | ||
| ); | ||
|
|
||
| const ConfidenceCellRenderer = ({ value }: { value: number }) => ( | ||
| <span | ||
| style={{ height: "100%", display: "flex", alignItems: "center" }} | ||
| >{`+/- ${value.toFixed(2)}%`}</span> | ||
| ); | ||
|
|
||
| const FeedCellRenderer = ({ value }: { value: string }) => ( | ||
| <div style={{ height: "100%", display: "flex", alignItems: "center" }}> | ||
| <SymbolPairTag | ||
| displaySymbol={value} | ||
| icon={<BtcIcon />} | ||
| description={value} | ||
| /> | ||
| </div> | ||
| ); | ||
|
|
||
| const FeedCellRendererLoading = () => ( | ||
| <div style={{ height: "100%", display: "flex", alignItems: "center" }}> | ||
| <SymbolPairTag isLoading /> | ||
| </div> | ||
| ); |
There was a problem hiding this comment.
Very interesting discussion to be had here as well regarding what the right abstractions are.
- Should
priceCellRendererinherit from anumberCellRenderer?- Perhaps there should even be a
priceColDef?- And perhaps there should even be a
priceColDefFactory, to which I can pass parameters likecolorizeNumbers?- ...and so on ad infinitum (this was only 3 layers of abstraction deep!)
- And perhaps there should even be a
- Perhaps there should even be a
There was a problem hiding this comment.
Yeah I think with these kind of things the best way is let things as simple as possible and then create the abstraction as needed, when we start duplicating the code or having a similar logic in 2-3 places.
| --ag-browser-color-scheme: light-dark(light, dark); | ||
| --ag-background-color: #{theme.color("background", "primary")}; | ||
| --ag-header-background-color: var(--ag-background-color); | ||
| --ag-foreground-color: #{theme.color("paragraph")}; | ||
| --ag-accent-color: #{theme.color("button", "outline", "background", "active")}; | ||
| --ag-header-font-size: #{theme.font-size("xs")}; | ||
| --ag-header-font-weight: #{theme.font-weight("medium")}; | ||
| --ag-cell-font-size: #{theme.font-size("xs")}; | ||
| --ag-wrapper-border: none; | ||
| --ag-cell-text-color: #{theme.color("paragraph")}; | ||
| --ag-data-font-size: #{theme.font-size("base")}; |
alexcambose
force-pushed
the
feat/table-grid
branch
from
440b045 to
42ca144
Compare
Summary
Adding an initial version of ag-grid based grid.
Rationale
How has this been tested?