-
Notifications
You must be signed in to change notification settings - Fork 11
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
feat(Table): new component #1129
Changes from 1 commit
fd86c17
be044ba
f4233f3
a98ca43
df41437
92c108a
af45ccf
f621e4c
e8d7bf3
39285a1
07598f6
91da2b2
5ee2705
2383126
cce55e0
cfab953
9004f64
4bd356a
ca1d352
aedfd55
a4c2cca
6346e75
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
marcoskolodny marked this conversation as resolved.
Show resolved
Hide resolved
marcoskolodny marked this conversation as resolved.
Show resolved
Hide resolved
|
yceballost marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I know this is a wrong usage, but if you write a string in columnTextAlign prop, the alignment changes for each letter. That's weird and I don't know if it is something to avoid columnTextAlign.-.Table.-.bug.mov
yceballost marked this conversation as resolved.
Show resolved
Hide resolved
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,9 +7,13 @@ import {InternalBoxed} from './boxed'; | |
import classNames from 'classnames'; | ||
import Box from './box'; | ||
import {applyCssVars} from './utils/css'; | ||
import ResponsiveLayout, {ResetResponsiveLayout} from './responsive-layout'; | ||
|
||
import type {DataAttributes} from './utils/types'; | ||
|
||
type TextAlign = 'left' | 'right' | 'center'; | ||
type VerticalAlign = 'top' | 'middle'; | ||
|
||
type TableProps = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
heading: Array<string>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you confirm if headers also support custom elements @aweell ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not according the spec, and I think it could be problematic, how would you show them in mobile (collapse-rows mode)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yep, I remember talking to Alex about this but I don't remember what the outcome was. I thought that the custom item would be included and that in mobile it is the product's responsibility... that's why I wanted Alex's confirmation. The product could use strings in mobile and custom composition in desktop, for example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That was the idea yes, allow slots in headers mostly for desktop views. It was changed in a small paragraph in the desktop behaviour but not in the anatomy, is already fixed in the specs There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
content?: Array<Array<React.ReactNode>>; | ||
|
@@ -20,7 +24,8 @@ type TableProps = { | |
* render every row as a card | ||
*/ | ||
responsive?: 'scroll' | 'collapse-rows'; | ||
columnTextAlign?: Array<'left' | 'right' | 'center'>; | ||
columnTextAlign?: Array<TextAlign> | TextAlign; | ||
rowVerticalAlign?: VerticalAlign; | ||
columnWidth?: Array<number | string>; | ||
/** | ||
* by default, the table expands to all the available width, if you want the table to have the minimum width to fit the rows content, set fullWidth to false. | ||
|
@@ -51,11 +56,23 @@ export const Table = React.forwardRef( | |
maxHeight, | ||
emptyCase, | ||
columnTextAlign, | ||
rowVerticalAlign = 'middle', | ||
columnWidth, | ||
...otherProps | ||
}: TableProps, | ||
ref: React.Ref<HTMLDivElement> | ||
) => { | ||
const getColumnTextAlign = (column: number) => { | ||
const defaultTextAlign = 'left'; | ||
if (columnTextAlign) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps you can use a default parameter to simplify this to: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. not really, it whould be something like: Array.isArray(columnTextAlign) ? columnTextAlign[column] ?? defaultTextAlign : columnTextAlign; There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see, that's in case the user just sends the textAlign for some of the columns... but given input parameter is an array and not something like a map[columnIdx->alignment]... is that a good use of the API? 🤔 |
||
if (Array.isArray(columnTextAlign)) { | ||
return columnTextAlign[column] ?? defaultTextAlign; | ||
} | ||
return columnTextAlign; | ||
} | ||
return defaultTextAlign; | ||
}; | ||
|
||
const collapsedRowsMode = responsive === 'collapse-rows'; | ||
const table = ( | ||
<table | ||
|
@@ -80,7 +97,10 @@ export const Table = React.forwardRef( | |
<th | ||
scope="col" | ||
key={idx} | ||
className={styles.cellTextAlign[columnTextAlign?.[idx] ?? 'left']} | ||
className={classNames( | ||
styles.cellTextAlign[getColumnTextAlign(idx)], | ||
styles.verticalAlign[rowVerticalAlign] | ||
)} | ||
style={{width: columnWidth?.[idx]}} | ||
> | ||
{header} | ||
|
@@ -90,10 +110,10 @@ export const Table = React.forwardRef( | |
</thead> | ||
<tbody> | ||
{content && content.length ? ( | ||
content.map((row, idx) => ( | ||
<tr key={idx}> | ||
content.map((row, rowIdx) => ( | ||
<tr key={rowIdx}> | ||
{row.map((cell, idx) => ( | ||
<td key={idx}> | ||
<td key={idx} className={styles.verticalAlign[rowVerticalAlign]}> | ||
{/** | ||
* In collapsedRowsMode, we render the row heading text before every cell content, except for the first cell | ||
* of every row, which is rendered with a medium weight font, as it's the row title. | ||
|
@@ -114,7 +134,7 @@ export const Table = React.forwardRef( | |
> | ||
<div | ||
className={classNames( | ||
styles.cellTextAlign[columnTextAlign?.[idx] ?? 'left'], | ||
styles.cellTextAlign[getColumnTextAlign(idx)], | ||
{ | ||
[styles.collapsedRowTittle]: | ||
idx === 0 && collapsedRowsMode, | ||
|
@@ -169,16 +189,27 @@ export const Table = React.forwardRef( | |
dataAttributes={{'component-name': 'Table', ...dataAttributes}} | ||
> | ||
<div {...scrollContainerStyles}> | ||
<Box paddingX={{desktop: 8, mobile: collapsedRowsMode ? 0 : 8}}>{table}</Box> | ||
<Box | ||
paddingX={{desktop: 16, mobile: collapsedRowsMode ? 0 : 8}} | ||
paddingY={{desktop: 8, mobile: 0}} | ||
> | ||
{table} | ||
</Box> | ||
</div> | ||
</InternalBoxed> | ||
); | ||
} | ||
|
||
return ( | ||
<div ref={ref} {...getPrefixedDataAttributes(dataAttributes, 'Table')} {...scrollContainerStyles}> | ||
{table} | ||
</div> | ||
<ResetResponsiveLayout> | ||
<div | ||
ref={ref} | ||
{...getPrefixedDataAttributes(dataAttributes, 'Table')} | ||
{...scrollContainerStyles} | ||
> | ||
<ResponsiveLayout>{table}</ResponsiveLayout> | ||
</div> | ||
</ResetResponsiveLayout> | ||
); | ||
} | ||
); |
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.
Visually stunning