-
Notifications
You must be signed in to change notification settings - Fork 28
Show middle state badges for running, starred and checked sub-rows #2004
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,11 +1,13 @@ | ||
| import React from 'react' | ||
| import cx from 'classnames' | ||
| import { VSCodeCheckbox } from '@vscode/webview-ui-toolkit/react' | ||
| import { Indicator, IndicatorWithJustTheCounter } from './Indicators' | ||
| import styles from './styles.module.scss' | ||
| import { CellProp, RowProp } from './interfaces' | ||
| import ClockIcon from '../../../shared/components/icons/Clock' | ||
| import { clickAndEnterProps } from '../../../util/props' | ||
| import { StarFull, StarEmpty } from '../../../shared/components/icons' | ||
| import { pluralize } from '../../../util/strings' | ||
|
|
||
| const RowExpansionButton: React.FC<RowProp> = ({ row }) => | ||
| row.canExpand ? ( | ||
|
|
@@ -31,27 +33,115 @@ const RowExpansionButton: React.FC<RowProp> = ({ row }) => | |
| <span className={styles.rowArrowContainer} /> | ||
| ) | ||
|
|
||
| export const FirstCell: React.FC< | ||
| CellProp & { | ||
| bulletColor?: string | ||
| isRowSelected: boolean | ||
| toggleExperiment: () => void | ||
| toggleRowSelection: () => void | ||
| toggleStarred: () => void | ||
| export type RowActionsProps = { | ||
| isRowSelected: boolean | ||
| showSubRowStates: boolean | ||
| starred?: boolean | ||
| subRowStates: { | ||
| selections: number | ||
| stars: number | ||
| plotSelections: number | ||
| } | ||
| > = ({ | ||
| cell, | ||
| bulletColor, | ||
| toggleRowSelection: () => void | ||
| toggleStarred: () => void | ||
| } | ||
|
|
||
| export type RowActionProps = { | ||
| showSubRowStates: boolean | ||
| subRowsAffected: number | ||
| actionAppliedLabel: string | ||
| children: React.ReactElement | ||
| hidden?: boolean | ||
| testId?: string | ||
| } | ||
|
|
||
| export const RowAction: React.FC<RowActionProps> = ({ | ||
| showSubRowStates, | ||
| subRowsAffected, | ||
| actionAppliedLabel, | ||
| children, | ||
| hidden, | ||
| testId | ||
| }) => { | ||
| const count = (showSubRowStates && subRowsAffected) || 0 | ||
|
|
||
| return ( | ||
| <div | ||
| className={cx(styles.rowActions, hidden && styles.hidden)} | ||
| data-testid={testId} | ||
| > | ||
| <Indicator | ||
| count={count} | ||
| tooltipContent={ | ||
| count && | ||
| `${count} ${pluralize('sub-row', count)} ${actionAppliedLabel}.` | ||
| } | ||
| > | ||
| {children} | ||
|
Contributor
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] At the moment only the direct children are being aggregated. Here is a screen recording of an example: Screen.Recording.2022-07-12.at.8.30.37.pm.movWhen we aggregate from checkpoints to experiments and then up to the current commit we are not taking the cumulative total into account. Another example: Screen.Recording.2022-07-12.at.8.35.31.pm.movI would not even try to include the top level indicator until we have more than one commit/branch in the table. Until that time we will be catering to an edge case. However, if you feel that they are needed then they need to reflect the totals including all children.
Contributor
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. More complicated aggregation behaviour will also need some tests.
Contributor
Author
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'll add some tests for this current use-case now, but I want to defer this total aggregation for later if it's currently an edge case. |
||
| </Indicator> | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| export const RowActions: React.FC<RowActionsProps> = ({ | ||
| isRowSelected, | ||
| toggleExperiment, | ||
| showSubRowStates, | ||
| starred, | ||
| subRowStates: { selections, stars }, | ||
| toggleRowSelection, | ||
| toggleStarred | ||
| }) => { | ||
| return ( | ||
| <> | ||
| <RowAction | ||
| showSubRowStates={showSubRowStates} | ||
| subRowsAffected={selections} | ||
| actionAppliedLabel={'selected'} | ||
| testId={'row-action-checkbox'} | ||
| > | ||
| <VSCodeCheckbox | ||
| {...clickAndEnterProps(toggleRowSelection)} | ||
| checked={isRowSelected} | ||
| /> | ||
| </RowAction> | ||
| <RowAction | ||
| showSubRowStates={showSubRowStates} | ||
| subRowsAffected={stars} | ||
| actionAppliedLabel={'starred'} | ||
| testId={'row-action-star'} | ||
| > | ||
| <div | ||
| className={styles.starSwitch} | ||
| role="switch" | ||
| aria-checked={starred} | ||
| tabIndex={0} | ||
| {...clickAndEnterProps(toggleStarred)} | ||
| data-testid="star-icon" | ||
| > | ||
| {starred && <StarFull />} | ||
| {!starred && <StarEmpty />} | ||
| </div> | ||
| </RowAction> | ||
| </> | ||
| ) | ||
| } | ||
|
|
||
| export const FirstCell: React.FC< | ||
| CellProp & | ||
| RowActionsProps & { | ||
| bulletColor?: string | ||
| toggleExperiment: () => void | ||
| } | ||
| > = ({ cell, bulletColor, toggleExperiment, ...rowActionsProps }) => { | ||
| const { row, isPlaceholder } = cell | ||
| const { | ||
| original: { starred, queued } | ||
| original: { queued } | ||
| } = row | ||
|
|
||
| const { | ||
| subRowStates: { plotSelections } | ||
| } = rowActionsProps | ||
|
|
||
| return ( | ||
| <div | ||
| {...cell.getCellProps({ | ||
|
|
@@ -63,29 +153,21 @@ export const FirstCell: React.FC< | |
| })} | ||
| > | ||
| <div className={styles.innerCell}> | ||
| <div className={styles.rowActions}> | ||
| <VSCodeCheckbox | ||
| {...clickAndEnterProps(toggleRowSelection)} | ||
| checked={isRowSelected} | ||
| /> | ||
| <div | ||
| className={styles.starSwitch} | ||
| role="switch" | ||
| aria-checked={starred} | ||
| tabIndex={0} | ||
| {...clickAndEnterProps(toggleStarred)} | ||
| data-testid="star-icon" | ||
| > | ||
| {starred && <StarFull />} | ||
| {!starred && <StarEmpty />} | ||
| </div> | ||
| </div> | ||
| <RowActions {...rowActionsProps} /> | ||
| <RowExpansionButton row={row} /> | ||
| <span | ||
| className={styles.bullet} | ||
| style={{ color: bulletColor }} | ||
| {...clickAndEnterProps(toggleExperiment)} | ||
| > | ||
| <IndicatorWithJustTheCounter | ||
| aria-label={'Sub-rows selected for plots'} | ||
| count={plotSelections} | ||
| tooltipContent={`${plotSelections} ${pluralize( | ||
| 'sub-row', | ||
| plotSelections | ||
| )} selected for plots.`} | ||
| /> | ||
| {queued && <ClockIcon />} | ||
| </span> | ||
| {isPlaceholder ? null : ( | ||
|
|
||
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] A more central location for these util will be helpful.
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.
Splitting out utils will help to start reducing the size of this file and would give a central location for people to look for test building blocks.
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.
fine to create a separate PR for that