Skip to content
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.

Sign up for GitHub

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

Table fix #147

Merged
merged 8 commits into from
Jan 16, 2024
Merged

Table fix #147

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
59af634
use correct theme hook
Jan 16, 2024
afeece3
custom TDs and THs
Jan 16, 2024
45558cb
cleanup
Jan 16, 2024
c3f93b3
some sorting working
Jan 16, 2024
fbe5a8d
sticky cell
Jan 16, 2024
426e5b6
sorting with style
Jan 16, 2024
978ccec
rename var
Jan 16, 2024
804d624
last index style
Jan 16, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
170 changes: 132 additions & 38 deletions ...app/(post-login)/submission/program/[shortName]/clinical-data/ClinicalEntityDataTable.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -475,10 +475,12 @@ const ClinicalEntityDataTable = ({
.sort(sortEntityData);
}

const styleThickBorderString = `3px solid ${theme.colors.grey}`;
const getHeaderBorder = (key) =>
(showCompletionStats && key === completionColumnHeaders.followUps) ||
(!showCompletionStats && key === 'donor_id')
? `3px solid ${theme.colors.grey}`
(!showCompletionStats && key === 'donor_id') ||
key === 'FO'
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's FO in this context? may be useful to abstract this into a declaratively named constant, to facilitate reading.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's logic like this all over this file.

? styleThickBorderString
: '';

const [stickyDonorIDColumnsWidth, setStickyDonorIDColumnsWidth] = useState(74);
Expand Down Expand Up @@ -636,71 +638,159 @@ const ClinicalEntityDataTable = ({
borderBottom: `1px solid ${theme.colors.grey_2}`,
};

const headerStyle = css`
background-color: ${theme.colors.grey_4};
border-bottom: 1px solid ${theme.colors.grey_2};
border-right: 1px solid ${theme.colors.grey_2};
padding: 5px;
font-size: 13px;
text-align: left;
`;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While I know it's a contentious point in the community, often labelled as "nit picking", I truly believe alphabetising properties does help preventing duplicates... which in CSS may introduce bugs that can be rather hard to find at times, because of how the order matters (keyword "cascading").

In this tiny list it may seem superfluous, but we should do it everywhere to elicit consistency, right?
broken windows, etc.


const stickyCSS = css`
position: sticky !important;
left: 0;
top: 0;
z-index: 1;
background-color: white;
`;

columns = [
{
id: 'clinical_core_completion_header',
header: (
<div
css={css`
display: flex;
align-items: center;
justify-content: center;
position: relative;
`}
>
CLINICAL CORE COMPLETION
<Tooltip
style={{ position: 'absolute', left: 'calc(100% - 20px)', top: '-2px' }}
html={
<p
css={css`
margin: 0px;
margin-right: 6px;
`}
>
For clinical completeness, each donor requires: <br />
DO: at least one Donor record <br />
PD: at least one Primary Diagnosis record <br />
NS: all the registered Normal DNA Specimen record <br />
TS: all the registered Tumour DNA Specimen record <br />
TR: at least one Treatment record <br />
FO: at least one Follow Up record <br />
</p>
}
meta: { customHeader: true },
sortingFn: sortEntityData,
header: (props) => {
return (
<th
colSpan={props.colSpan}
css={css`
${headerStyle};
border-right: 3px solid ${theme.colors.grey};
`}
>
<Icon name="question_circle" fill="primary_2" width="18px" height="18px" />
</Tooltip>
</div>
),
<div
css={css`
display: flex;
align-items: center;
justify-content: center;
position: relative;
`}
>
CLINICAL CORE COMPLETION
<Tooltip
style={{ position: 'absolute', left: 'calc(100% - 20px)', top: '-2px' }}
html={
<p
css={css`
margin: 0px;
margin-right: 6px;
`}
>
For clinical completeness, each donor requires: <br />
DO: at least one Donor record <br />
PD: at least one Primary Diagnosis record <br />
NS: all the registered Normal DNA Specimen record <br />
TS: all the registered Tumour DNA Specimen record <br />
TR: at least one Treatment record <br />
FO: at least one Follow Up record <br />
Copy link
Member

@justincorrigible justincorrigible Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not a request for change, because I know this is an established pattern and not something just from this PR

just a thought I had, on how we could do these non-dev business-domain things better for new hires who won't have the background or context (and me, inexcusably).
🤔 this dictionary right here is what I wish we do with VS' intellisense, that if you hover over a constant (like the one I suggested here), it also gives you a description of what it means, apart from its string value.
wonder if there's a plugin to do that sort of thing.

Copy link
Member

@justincorrigible justincorrigible Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

update:
ahh JSDoc to the rescue! no plugin needed 👍
image

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

</p>
}
>
<Icon name="question_circle" fill="primary_2" width="18px" height="18px" />
</Tooltip>
</div>
</th>
);
},
headerStyle: completionHeaderStyle,
columns: columns.slice(0, 7).map((column) => ({
...column,
sortingFn: sortEntityData,
header: (props) => {
const val = props.header.id;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

val -> value, no need to skimp on letters; unless value is already in scope, in which case aValue vs bValue would be better than variants of the same single word.

const isThickBorder = val === 'FO';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this one is thick border cause its the last element? Should the logic be based on it being the last element not a specific element?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's somewhat unclear in regards to the groupings in the table.
Last element requires us to target the last element of the row.

We can't access the row element.
So we'd have to do some logic with the indices from the internal react table data which we shouldn't be touching at this level. That is way more flaky than just hard coding a value.

DX wise val === "FO is very clear. It works and is very much in line with the code quality in the rest of this file

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds fine, it is clear whats happening. Just to complete my though there though, do we not have access the index form the .map? partial code example:

const coreCompletionColumnsCount = 7;
columns: columns.slice(0, coreCompletionColumnsCount).map((column, index) => ({
	...column,
	sortingFn: sortEntityData,
	header: (props) => {
		const isLastElement = index === coreCompletionColumnsCount -1;

const isSticky = val === 'donor_id';
const isSorted = props.sorted;

return (
<th
onClick={props.getSortingHandler()}
css={css`
padding: 2px 6px;
font-size: 12px;
:hover {
cursor: pointer;
}
border-bottom: 1px solid ${theme.colors.grey_2};
border-right: ${isThickBorder
? styleThickBorderString
: `1px solid ${theme.colors.grey_2}`};
${isSticky && stickyCSS}
${isSorted &&
`box-shadow: inset 0 ${isSorted === 'asc' ? '' : '-'}3px 0 0 rgb(7 116 211)`};
`}
>
{val}
</th>
);
},
maxWidth: noTableData ? 50 : 250,
style: noTableData ? noDataCellStyle : {},
meta: { customCell: true, customHeader: true },
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cool! hadn't seen this property in the react-table docs. or is it a uikit thing perhaps?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

more a uikit thing. otherwise the cell and header render functions get wrapped in specific components like <TableCell> and <TableHeaderWrapper>. This becomes problematic when you can't override styling they provide. For example padding on parent. You can hack around it but I opened a PR with that way of doing things and we decided as team to go the bigger more correct fix.

cell: (context) => {
const value = context.getValue();
const isSticky = context.column.id === 'donor_id';

const { isCompletionCell, errorState } = getCellStyles(
const { isCompletionCell, errorState, style } = getCellStyles(
undefined,
context.row,
context.column,
);

const showSuccessSvg = isCompletionCell && !errorState;

return showSuccessSvg ? (
const content = showSuccessSvg ? (
<Icon name="checkmark" fill="accent1_dimmed" width="12px" height="12px" />
) : (
value
);

return (
<td
css={css`
border-right: 1px solid ${theme.colors.grey_2};
${isSticky && stickyCSS}
`}
style={{
...style,
}}
>
<div
css={css`
font-size: 12px;
padding: 2px 8px;
min-width: 40px;
height: 28px;
`}
>
{content}
</div>
</td>
);
},
})),
},
{
id: 'submitted_donor_data_header',
header: <div>SUBMITTED DONOR DATA</div>,
meta: { customHeader: true },
header: (props) => (
<th colSpan={props.colSpan} css={headerStyle}>
<div>SUBMITTED DONOR DATA</div>
</th>
),
headerStyle: dataHeaderStyle,
columns: columns.slice(7).map((column, i) => column),
columns: columns.slice(7),
},
];
}
Expand Down Expand Up @@ -757,6 +847,7 @@ const ClinicalEntityDataTable = ({
/>
</div>
)}

<TableInfoHeaderContainer
left={
<Typography
Expand All @@ -776,6 +867,9 @@ const ClinicalEntityDataTable = ({
withSideBorders
withPagination
showPageSizeOptions
withStripes
enableColumnResizing={false}
enableSorting
Copy link
Member

@justincorrigible justincorrigible Jan 16, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in contrast, as follow up to my earlier "alphabetise all the things" comment, TS does a much better job at identifying and pointing out duplicate React props, so I don't care as much here.

...I'd only advocate for it here for consistency and "coding discipline", and I realise other devs don't feel as strongly about that as I do.

/>
</div>
);
Expand Down
3 changes: 1 addition & 2 deletions src/app/(post-login)/submission/program/[shortName]/clinical-data/page.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,7 @@ import useUrlParamState from '@/app/hooks/useUrlParamState';
import { notNull } from '@/global/utils';
import { css } from '@/lib/emotion';
import { useQuery } from '@apollo/client';
import { useTheme } from '@emotion/react';
import { Loader, Typography, VerticalTabs } from '@icgc-argo/uikit';
import { Loader, Typography, VerticalTabs, useTheme } from '@icgc-argo/uikit';
import { useState } from 'react';
import { setConfiguration } from 'react-grid-system';
import ClinicalEntityDataTable from './ClinicalEntityDataTable';
Expand Down