-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
refactor(SimpleTable): drop BEM, use Mantine TASK-1377 #5366
base: kalvis/mantine-setup
Are you sure you want to change the base?
refactor(SimpleTable): drop BEM, use Mantine TASK-1377 #5366
Conversation
…e-new-(mantine-poc)
…e-new-(mantine-poc)
…e-new-(mantine-poc)
jsapp/js/theme/kobo/Table.ts
Outdated
styles: (theme) => { | ||
return { | ||
table: { | ||
backgroundColor: theme.colors.gray[9], | ||
borderCollapse: 'separate', | ||
borderRadius: theme.radius.md, | ||
}, | ||
thead: { | ||
backgroundColor: theme.colors.gray[8], | ||
}, | ||
th: { | ||
fontSize: theme.fontSizes.sm, | ||
color: theme.colors.gray[2], | ||
fontWeight: '400', | ||
}, | ||
td: { | ||
fontSize: theme.fontSizes.md, | ||
borderTopWidth: '1px', | ||
borderTopColor: theme.colors.gray[7], | ||
borderTopStyle: 'solid', | ||
}, | ||
}; | ||
}, |
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.
let's prefer CSS Modules file over styles prop, because that edits CSS class once, instead of adding style="font-size: calc(0.875rem * var(--mantine-scale)); border-top: 1px solid rgb(237, 238, 242);"
to every cell individually (see DOM of storybook). Also, that's officially recommended.
Furthermore, would you agree to prefer vars prop over CSS Modules file, because that's a simple parameterization of the CSS Modules? Although performance is unknown and official docs lacks an opinion on using vars prop vs "hardcoded" values in CSS Modules.
<Text key='include-groups' ta='center'>{t('Include Groups')}</Text>, | ||
<Text key='multiple-versions' ta='center'>{t('Multiple Versions')}</Text>, |
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.
Looks OK to use <Text>
👌
{Array.from(this.getUniqueResponses()).map(this.renderResponseRow)} | ||
</bem.SimpleTable__body> | ||
</bem.SimpleTable> | ||
<Box mt='lg'> |
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'd prefer that mt='lg'
would be applicable to SimpleTable
itself. See suggestion over there
|
||
interface SimpleTableProps { | ||
head: TableData['head']; | ||
body: TableData['body']; | ||
/** | ||
* Passing minimum width enables contextual horizontal scrollbar (i.e. without | ||
* it the table will never display scrollbar - regardless of how small | ||
* the screen is). | ||
*/ | ||
minWidth?: number; | ||
} | ||
|
||
/** | ||
* A wrapper component for `Table` from `@mantine/core`. It requires column | ||
* headings, column data, and has optional minimum width. | ||
*/ | ||
export default function SimpleTable(props: SimpleTableProps) { | ||
const table = ( | ||
<Table | ||
data={{head: props.head, body: props.body}} | ||
horizontalSpacing='sm' | ||
verticalSpacing='sm' | ||
/> | ||
); | ||
|
||
if (props.minWidth) { | ||
return ( | ||
<Table.ScrollContainer minWidth={props.minWidth} type='native'> | ||
{table} | ||
</Table.ScrollContainer> | ||
); | ||
} | ||
|
||
return table; | ||
} |
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.
Let's support and pass-through style props, to avoid the need for <Box>
wrappers (see above)
interface SimpleTableProps { | |
head: TableData['head']; | |
body: TableData['body']; | |
/** | |
* Passing minimum width enables contextual horizontal scrollbar (i.e. without | |
* it the table will never display scrollbar - regardless of how small | |
* the screen is). | |
*/ | |
minWidth?: number; | |
} | |
/** | |
* A wrapper component for `Table` from `@mantine/core`. It requires column | |
* headings, column data, and has optional minimum width. | |
*/ | |
export default function SimpleTable(props: SimpleTableProps) { | |
const table = ( | |
<Table | |
data={{head: props.head, body: props.body}} | |
horizontalSpacing='sm' | |
verticalSpacing='sm' | |
/> | |
); | |
if (props.minWidth) { | |
return ( | |
<Table.ScrollContainer minWidth={props.minWidth} type='native'> | |
{table} | |
</Table.ScrollContainer> | |
); | |
} | |
return table; | |
} | |
interface SimpleTableProps extends MantineStyleProps { | |
head: TableData['head']; | |
body: TableData['body']; | |
/** | |
* Passing minimum width enables contextual horizontal scrollbar (i.e. without | |
* it the table will never display scrollbar - regardless of how small | |
* the screen is). | |
*/ | |
minWidth?: number; | |
} | |
/** | |
* A wrapper component for `Table` from `@mantine/core`. It requires column | |
* headings, column data, and has optional minimum width. | |
*/ | |
export default function SimpleTable({head, body, minWidth, ...styleProps}: SimpleTableProps) { | |
const table = ( | |
<Table | |
{...styleProps} | |
data={{head, body}} | |
horizontalSpacing='sm' | |
verticalSpacing='sm' | |
/> | |
); | |
if (minWidth) { | |
return ( | |
<Table.ScrollContainer minWidth={minWidth} type='native'> | |
{table} | |
</Table.ScrollContainer> | |
); | |
} | |
return table; | |
} | |
🗒️ Checklist
<type>(<scope>)<!>: <title> TASK-1234
frontend
orbackend
unless it's global📣 Summary
Replace
bem.SimpleTable
with Mantine-basedSimpleTable
component (a wrapper overTable
from@mantine/core
). The change for users is mainly visual.📖 Description
The tables that are affected by this are:
BulkEditSubmissionsForm
BulkEditRowForm
ProjectExportsList
SubmissionDataTable
(just responses for geopoint questions)💭 Notes
I based the looks of the
SimpleTable
on ourUniversalTable
as this is the latest design we have of "a table". This means it would look different from what we had, and it's ok.The steps I took to accomplish this Mantine based replacement:
js/components/common/SimpleTable.tsx
Table
from@mantine/core
<Table>
with some props passed to it (e.g.data
)js/components/common/SimpleTable.stories.tsx
js/theme/kobo/Table.ts
Table
from@mantine/core
export const TableThemeKobo = Table.extend({
withdefaultProps
changed andvars
changing CSS variable colorsborder-radius
to mySimpleTable
and changebackground-color
for headerTable
TableStylesNames
, and foundtable
:fanfare:js/theme/kobo/Table.ts
I've addedstyles.table
object toTableThemeKobo
and changed the style properties I wanted to change 👌border-collapse
soborder-radius
would work. Now I needed to fix the missing borders:thead
gets background colortd
gets border top stylesbem.SimpleTable
was pretty straightforward, some bumps on the road:Text
from Mantine wrapped around array items, rather than making this part ofSimpleTable
(this was my initial idea, but couldn't find a way to do it with Mantine without hacking things too much)Box
from Mantine, rather than having the optinal margins be part ofSimpleTable
(still not sure if this is the "proper" way of usingBox
)TableThemeKobo.styles
fromjs/theme/kobo/Table.ts
to CSS module injs/components/common/SimpleTable.module.scss
classNames
property on<Table>
<Box>
to apply margins, we passmt
toSimpleTable
directly. To make this work,SimpleTable
is acceptingMantineStyleProps
besides its own custom props.👀 Preview steps
Run Storybook and make sure that
SimpleTable
looks okVerify that
BulkEditSubmissionsForm
works correctlybeta
Verify that
BulkEditRowForm
works correctly8. ℹ️ follow all the above steps
9. for one of the rows click "edit" button from "Action" column
10. 🟢 notice a table with proposed existing responses appear
11. 🟢 verify it has the same functionalities as same table from
beta
Verify that
ProjectExportsList
works correctlybeta
Verify that
SubmissionDataTable
works correctly