refactor: optimize re-renders #87
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,130 @@ | ||
| import i18n from '@dhis2/d2-i18n' | ||
| import { TableRowHead, TableCellHead } from '@dhis2/ui' | ||
| import cx from 'classnames' | ||
| import PropTypes from 'prop-types' | ||
| import React from 'react' | ||
| import { useMetadata, selectors } from '../../metadata/index.js' | ||
| import { useActiveCell } from '../data-entry-cell/index.js' | ||
| import styles from './category-combo-table.module.css' | ||
| import { PaddingCell } from './padding-cell.js' | ||
| import { TotalHeader } from './total-cells.js' | ||
|
|
||
| export const CategoryComboTableHeader = ({ | ||
| dataElements, | ||
| renderRowTotals, | ||
| paddingCells, | ||
| categoryOptionCombos, | ||
| categories, | ||
| }) => { | ||
| const { data: metadata } = useMetadata() | ||
| const { deId: activeDeId, cocId: activeCocId } = useActiveCell() | ||
|
|
||
| // Computes the span and repeats for each columns in a category-row. | ||
| // Repeats are the number of times the options in a category needs to be rendered per category-row | ||
| let catColSpan = categoryOptionCombos.length | ||
| const rowToColumnsMap = categories.map((c) => { | ||
| const categoryOptions = selectors.getCategoryOptionsByCategoryId( | ||
| metadata, | ||
| c.id | ||
| ) | ||
| const nrOfOptions = c.categoryOptions.length | ||
| // catColSpan should always be equal to nrOfOptions in last iteration | ||
| // unless anomaly with categoryOptionCombo-generation server-side | ||
| if (nrOfOptions > 0 && catColSpan >= nrOfOptions) { | ||
| // calculate colSpan for current options | ||
| // this is the span for each option, not the "total" span of the row | ||
| catColSpan = catColSpan / nrOfOptions | ||
| // when table have multiple categories, options need to be repeated for each disaggregation "above" current-category | ||
| const repeat = | ||
| categoryOptionCombos.length / (catColSpan * nrOfOptions) | ||
|
|
||
| const columnsToRender = new Array(repeat) | ||
| .fill(0) | ||
| .flatMap(() => categoryOptions) | ||
|
|
||
| return { | ||
| span: catColSpan, | ||
| columns: columnsToRender, | ||
| category: c, | ||
| } | ||
| } else { | ||
| console.warn( | ||
| `Category ${c.displayFormName} malformed. Number of options: ${nrOfOptions}, span: ${catColSpan}` | ||
| ) | ||
| } | ||
| return c | ||
| }) | ||
|
|
||
| // Is the active cell in this cat-combo table? Check to see if active | ||
| // data element is in this CCT | ||
| const isThisTableActive = dataElements.some(({ id }) => id === activeDeId) | ||
|
|
||
| // Find if this column header includes the active cell's column | ||
| const isHeaderActive = (headerIdx, headerColSpan) => { | ||
| const activeCellColIdx = categoryOptionCombos.findIndex( | ||
| (coc) => activeCocId === coc.id | ||
| ) | ||
| const idxDiff = activeCellColIdx - headerIdx * headerColSpan | ||
| return isThisTableActive && idxDiff < headerColSpan && idxDiff >= 0 | ||
| } | ||
|
There was a problem hiding this comment. I was just wondering why we also need to Look at this very naively, I would expect that every cell will be unique so we don't need this variable per se. If you have added that Having said all of that, if you agree with my earlier suggestion, about moving the active/inactive logic down to the actual header cell, then this code is going to change anyway. There was a problem hiding this comment. I did some refactoring to not need to pass all
No, and that's why this fact is used to check if the current table is "active" or not.
I don't really understand what you mean here? This is only a part of the check There was a problem hiding this comment.
Partly from your explanation above, and by looking at the code a bit more closely, I get this now 👍 .
I was mainly just figuring out what was happening, and my suggestion about returning early was basically just this (see the comment): const isHeaderActive = (headerIdx, headerColSpan) => {
if (!checkTableActive(activeDeId)) {
return false
}
// by returning early you can avoid this iteration
const activeCellColIdx = categoryOptionCombos.findIndex(
(coc) => activeCocId === coc.id
)
const idxDiff = activeCellColIdx - headerIdx * headerColSpan
return (idxDiff < headerColSpan && idxDiff >= 0)
}But this is very much a micro-optimisation, so I'm very happy to skip this. |
||
|
|
||
| return rowToColumnsMap.map((colInfo, colInfoIndex) => { | ||
| const { span, columns, category } = colInfo | ||
| return ( | ||
| <TableRowHead key={category.id}> | ||
| <TableCellHead | ||
| className={styles.categoryNameHeader} | ||
| colSpan={'1'} | ||
| > | ||
| {category.displayFormName !== 'default' && | ||
| category.displayFormName} | ||
| </TableCellHead> | ||
| {columns.map((co, columnIndex) => { | ||
| return ( | ||
| <TableCellHead | ||
| key={columnIndex} | ||
| className={cx(styles.tableHeader, { | ||
| [styles.active]: isHeaderActive( | ||
| columnIndex, | ||
| span | ||
| ), | ||
| })} | ||
| colSpan={span.toString()} | ||
| > | ||
| {co.isDefault | ||
| ? i18n.t('Value') | ||
| : co.displayFormName} | ||
| </TableCellHead> | ||
| ) | ||
| })} | ||
| {paddingCells.map((_, i) => ( | ||
| <PaddingCell key={i} /> | ||
| ))} | ||
| {renderRowTotals && colInfoIndex === 0 && ( | ||
| <TotalHeader rowSpan={categories.length} /> | ||
| )} | ||
| </TableRowHead> | ||
| ) | ||
| }) | ||
| } | ||
|
|
||
| CategoryComboTableHeader.propTypes = { | ||
| categoryCombo: PropTypes.shape({ | ||
| id: PropTypes.string.isRequired, | ||
| categoryOptionCombos: PropTypes.array, | ||
| }).isRequired, | ||
| categories: PropTypes.array, | ||
| categoryOptionCombos: PropTypes.array, | ||
| dataElements: PropTypes.arrayOf( | ||
| PropTypes.shape({ | ||
| id: PropTypes.string.isRequired, | ||
| categoryCombo: PropTypes.shape({ | ||
| id: PropTypes.string, | ||
| }), | ||
| displayFormName: PropTypes.string, | ||
| valueType: PropTypes.string, | ||
| }) | ||
| ), | ||
| paddingCells: PropTypes.array, | ||
| renderRowTotals: PropTypes.bool, | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| import { TableCell } from '@dhis2/ui' | ||
| import cx from 'classnames' | ||
| import PropTypes from 'prop-types' | ||
| import React from 'react' | ||
| import { useActiveCell } from '../data-entry-cell/index.js' | ||
| import styles from './category-combo-table.module.css' | ||
|
|
||
| export const DataElementCell = ({ dataElement }) => { | ||
| const { deId: activeDeId } = useActiveCell() | ||
| return ( | ||
| <TableCell | ||
| className={cx(styles.dataElementName, { | ||
| [styles.active]: dataElement.id === activeDeId, | ||
| })} | ||
| > | ||
| {dataElement.displayFormName} | ||
| </TableCell> | ||
| ) | ||
| } | ||
|
|
||
| DataElementCell.propTypes = { | ||
| dataElement: PropTypes.shape({ | ||
| id: PropTypes.string.isRequired, | ||
| categoryCombo: PropTypes.shape({ | ||
| id: PropTypes.string, | ||
| }), | ||
| displayFormName: PropTypes.string, | ||
| valueType: PropTypes.string, | ||
| }), | ||
| } | ||
|
|
||
| export default DataElementCell |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| import { TableCell } from '@dhis2/ui' | ||
| import React from 'react' | ||
| import styles from './category-combo-table.module.css' | ||
|
|
||
| export const PaddingCell = () => ( | ||
| <TableCell className={styles.paddingCell}></TableCell> | ||
| ) | ||
|
|
||
| export default PaddingCell |
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'm thinking that it would probably benefit the app's architecture and performance if we were to push this form state subscription down to the level of the actual header cell.
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 don't really think it's necessary to push it further. You could in theory have a
CategoryTableHeaderColumnsand move it there, and basically move the rendered component fromcolumns.map.... But that would only save re-rendering of the static components "Category name" and "PaddingCell". Again these are static, with no dynamic props and no DOM-updated are needed.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 was mainly concerned about the nested iterations needed to compute the columns, and that fact that they would need to run whenever a the active cell changes. I suggested solving this by pushing the subscription to the active cell down, but I noticed you are now memoizing these columns in the
useCategoryColumns. I think that's an equally valid solution.