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

Jump to bottom

feat(table): adds single row selection mode for data table #888

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

feat(table): adds single row selection mode for data table #888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c459a12
feat(table): adds single row selection mode for data table
ogermain-kronos May 28, 2024
68863f9
feat(table): eslint
ogermain-kronos May 28, 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
26 changes: 21 additions & 5 deletions packages/react/src/components/table/table.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -188,7 +188,7 @@ describe('Table', () => {
const callback = jest.fn();
const wrapper = mountWithTheme(
<Table
selectableRows
selectionMode="multiple"
columns={columns}
data={data}
onRowClick={callback}
Expand All @@ -205,7 +205,7 @@ describe('Table', () => {

mountWithTheme(
<Table
selectableRows
selectionMode="multiple"
columns={columns}
data={data}
onSelectedRowsChange={callback}
Expand All @@ -220,7 +220,7 @@ describe('Table', () => {
const callback = jest.fn();
const wrapper = mountWithTheme(
<Table
selectableRows
selectionMode="multiple"
columns={columns}
data={data}
onSelectedRowsChange={callback}
Expand All @@ -236,7 +236,7 @@ describe('Table', () => {
const callback = jest.fn();
const wrapper = mountWithTheme(
<Table
selectableRows
selectionMode="multiple"
columns={columns}
data={data}
onSelectedRowsChange={callback}
Expand Down Expand Up @@ -309,7 +309,7 @@ describe('Table', () => {
});

test('has selectable rows styles', () => {
const tree = renderWithProviders(<Table selectableRows columns={columns} data={data} />);
const tree = renderWithProviders(<Table selectionMode="multiple" columns={columns} data={data} />);

expect(tree).toMatchSnapshot();
});
Expand Down Expand Up @@ -347,4 +347,20 @@ describe('Table', () => {

expect(tree).toMatchSnapshot();
});

test('has radio buttons in single selection mode', () => {
const callback = jest.fn();
const wrapper = mountWithTheme(
<Table
selectionMode="single"
columns={columns}
data={data}
onSelectedRowsChange={callback}
/>,
);

expect(getByTestId(wrapper, 'row-radiobutton-0')
.find('input')
.prop('type')).toBe('radio');
});
});
76 changes: 51 additions & 25 deletions packages/react/src/components/table/table.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@ type RowSize = 'small' | 'medium';

type UtilityColumnType = 'selection' | 'numbers' | 'expand';

type SelectionMode = 'single' | 'multiple';

function getThPadding(device: DeviceType, rowSize?: RowSize): string {
if (rowSize === 'small') {
switch (device) {
Expand Down Expand Up @@ -140,31 +142,55 @@ const ExpandButton = styled(IconButton) <{ $expanded: boolean }>`
}
`;

function getUtilityColumn<T extends object>(type: UtilityColumnType, t: TFunction<'translation'>): TableColumn<T> {
const RadioButton = styled.input`
Copy link
Contributor

@savutsang savutsang May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ca n'a pas le design du Radio Button

Actuel image

Expected image

On a deja ca dans Radio-Button-Group, il faudrait comme extraire juste le input pis faire un component Radio Button seul.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

On s'inspire du King, I see
image

margin: 3px 3px 3px 5px;
vertical-align: inherit;
`;

function getUtilityColumn<T extends object>(
type: UtilityColumnType,
t: TFunction<'translation'>,
selectionMode?: SelectionMode,
): TableColumn<T> {
const column: TableColumn<T> = {
id: type,
className: utilColumnClassName,
};

switch (type) {
case 'selection':
column.header = ({ table }) => (
<Checkbox
data-testid="row-checkbox-all"
checked={table.getIsAllRowsSelected()}
indeterminate={table.getIsSomeRowsSelected()}
onChange={table.getToggleAllRowsSelectedHandler()}
/>
);
column.cell = ({ row }) => (
<Checkbox
data-testid={`row-checkbox-${row.index}`}
checked={row.getIsSelected()}
disabled={!row.getCanSelect()}
indeterminate={row.getIsSomeSelected()}
onChange={row.getToggleSelectedHandler()}
/>
);
if (selectionMode === 'multiple') {
column.header = ({ table }) => (
<Checkbox
data-testid="row-checkbox-all"
checked={table.getIsAllRowsSelected()}
indeterminate={table.getIsSomeRowsSelected()}
onChange={table.getToggleAllRowsSelectedHandler()}
/>
);
column.cell = ({ row }) => (
<Checkbox
data-testid={`row-checkbox-${row.index}`}
checked={row.getIsSelected()}
disabled={!row.getCanSelect()}
indeterminate={row.getIsSomeSelected()}
onChange={row.getToggleSelectedHandler()}
/>
);
} else if (selectionMode === 'single') {
column.cell = ({ table, row }) => (
<RadioButton
Copy link
Contributor

Choose a reason for hiding this comment

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

La radio button devrait avoir un <label> d'associé. S'il n'y a pas de <label> les technologies d'assistances (lecteur d'écran) vont seulement annoncer que c'est un radio button et rien d'autre. Le <label> sert à identifier de quel radio button il s'agit. e.g., si le <label> est 'patate' le lecteur d'écran va annoncer: 'patate, radio button'
Il y a deux options possibles:

  1. aria-label
    ajouter un aria-label="label du radio button" à l'input.
    Tu aurais donc:
    <input data-testid="..." type="radio" class="..." aria-label="patate">.

  2. hidden text
    ajouter une classe qui vient cacher visuellement le <label>.
    <input id="idInput" data-testid="..." type="radio" class="..."><label for="idInput" class="visually-hidden">patate</label>

Comme mentionné dans un autre commentaire, il faudrait utiliser le composant de radio button du DS. Ça va p-e demander du boulot dans le composant radio-button par contre.
Je vais regarder le travaille qu'il y a à faire sur le composant et si c'est trop de boulot, je vais créer une carte pour y faire les modifications et on reviendra sur ta PR quand les modifications pour le radio button seront fait.

data-testid={`row-radiobutton-${row.index}`}
type="radio"
checked={row.getIsSelected()}
disabled={!row.getCanSelect()}
onChange={row.getToggleSelectedHandler()}
onClick={() => {
table.toggleAllRowsSelected(false);
Copy link
Contributor

@savutsang savutsang May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

J'ouvre la discussion, mais le radio button a un comportement de toggle presentement, si tu le re-click, ca va le deselectionner. Et je ne sais pas si c'est voulu ou conforme. Dans mon experience je n'ai jamais vu un radio qui peut se deselectionner. En meme temps, on a pas d'autre moyen d'enlever une selection non plus. C'est quoi vos opinions la dessus?

@LarryMatte @Genmoree

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Je l'ai fait comme ça parce que j'étais pas certain qu'on devait locker le user dans un choix ou non. Je suis ouvert aux propositions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Pas une réponse concrète mais un peu de recherche.

Les exemples que j'ai trouvés pour un single select avec bouton radio n'offraient pas de moyen de désélectionner:
https://www.lightningdesignsystem.com/components/data-tables/#Single-row-selection
https://www.naiveui.com/en-US/os-theme/components/data-table#select-single.vue

Celle-là le permet mais elle n'a pas de bouton radio. Ça retire l'ambiguité mais ça communique moins bien la possibilité d'interaction selon moi:
https://datatables.net/examples/api/select_single_row.html

Copy link
Contributor

@savutsang savutsang May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah le datatables.net m'apparait pas trop clair, j'ai pas l'impression de vraiment selectionner le row. Ca peut confondre avec un action de juste highlighter un row pour faciliter la lecture.

Comment on lines +188 to +189
Copy link
Contributor

@WilliamsTardif WilliamsTardif May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

J'ai testé la nouvelle fonctionnalité et dite moi si je sors du scope de la tâche.
Pas que j'ai le parkinson, mais j'ai trouvé ça un peu désagréable de devoir être précis pour clicker sur le bouton radio. Est-ce qui serait pas trop compliquer de toggle quand on click aussi la row ? 🤔
Sa peu-être déjà été discuté, mais je pose quand même la question au cas où. 🤷‍♂️

Copy link
Contributor

@LarryMatte LarryMatte May 31, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

J'aime vraiment la réflexion.
Par contre, disons que la row est clickable au complet:

  • s'il y a une cell avec un lien <a> à l'intérieur, ça peut être tannant lorsque l'utilisateur va clicker sur le lien, ca va trigger le radio button, même chose s'il y a des <button>.
  • si l'utilisateur veut sélectionner le texte d'une cell pour faire un copier/coller, il va devoir clicker dans la cell pour le sélectionner et ca va trigger le radio button.

Je sais que nous pourrions contourner tout ça d'une manière xyz mais je préfère garder ca le plus simple possible. Ce que nous pourrions faire par contre serait d'avoir la cell clickable au lieu de toute la row.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 pour le cell cliquable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Comme on s'enligne pour créer notre propre component pour le radio button, il devrait inclure un élément label comme la checkbox. À ce moment-là, il serait possible d'ajouter un padding au label (même sans texte) pour grossir le hit area. Ça éviterait de faire une passe-passe avec le clic sur la cell.

Je ne pense pas que le label soit un problème pour l'a11y parce que les attribut aria-labelledby ont la priorité sur les éléments <label> lorsque les deux sont présents.

}}
/>
);
}
break;

case 'numbers':
Expand Down Expand Up @@ -214,7 +240,7 @@ export interface TableProps<T extends object> {
* @default medium
*/
rowSize?: RowSize;
selectableRows?: boolean;
selectionMode?: SelectionMode;
Copy link
Contributor

@savutsang savutsang May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

C'est un BREAKING CHANGE, il faut ajouter un ! et documenter le breaking change.

BREAKING CHANGE: 

- Table prop "selectableRows" is not a boolean anymore.

https://www.conventionalcommits.org/en/v1.0.0/#commit-message-with-both--and-breaking-change-footer

Copy link
Contributor

@savutsang savutsang May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

On a aussi un expandableRows?: 'single' | 'multiple', le concept est similaire, on devrait garder la meme nomenclature.

Suggested change
selectionMode?: SelectionMode;
selectableRows?: SelectionMode;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

C'est débatable. Honnnêtement je trouve que selectionMode est pas mal plus clair sur la fonction de la prop que selectableRows imho.

Copy link
Contributor

@savutsang savutsang May 30, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah ton point est valide, pour moi c'est pas tant le nom mais plus le fait qu'on pourrait garder une certaine similitude entre les noms. Ca pourrait etre expandableRows qui se renomme a expandMode aussi.

Copy link
Contributor

Choose a reason for hiding this comment

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

Par contre je me demande si on ne devrait pas garder row ou rows dans les noms de props, vu que ça affecte les rows et non la table. row(s)Selection et row(s)Expansion?

/**
* Adds striped rows
* @default false
Expand All @@ -239,7 +265,7 @@ export const Table = <T extends object>({
stickyFooter = false,
rowNumbers = false,
rowSize = 'medium',
selectableRows,
selectionMode,
striped = false,
manualSort = false,
onRowClick,
Expand All @@ -257,16 +283,16 @@ export const Table = <T extends object>({
const columns = useMemo(() => {
const cols = [...providedColumns];

if (selectableRows) {
cols.unshift(getUtilityColumn<T>('selection', t));
if (selectionMode) {
cols.unshift(getUtilityColumn<T>('selection', t, selectionMode));
} else if (expandableRows) {
cols.unshift(getUtilityColumn<T>('expand', t));
} else if (rowNumbers) {
cols.unshift(getUtilityColumn<T>('numbers', t));
}

return cols;
}, [selectableRows, expandableRows, rowNumbers, providedColumns, t]);
}, [selectionMode, expandableRows, rowNumbers, providedColumns, t]);

const tableOptions: TableOptions<T> = {
data,
Expand Down Expand Up @@ -311,13 +337,13 @@ export const Table = <T extends object>({
const currentRowSelection = table.getState().rowSelection;

useEffect(() => {
if (selectableRows && onSelectedRowsChange) {
if (selectionMode && onSelectedRowsChange) {
const selectedRowIds = currentRowSelection;
const selectedIndexes = Object.keys(selectedRowIds).filter((index) => selectedRowIds[index]);
const selectedRows = selectedIndexes.map((index) => data[parseInt(index, 10)]);
onSelectedRowsChange(selectedRows);
}
}, [selectableRows, currentRowSelection, onSelectedRowsChange, data]);
}, [selectionMode, currentRowSelection, onSelectedRowsChange, data]);

return (
<StyledTable
Expand Down
45 changes: 42 additions & 3 deletions packages/storybook/stories/table.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -534,7 +534,7 @@ export const SortableRows: Story = () => {
);
};

export const SelectableRows: Story = () => {
export const MultipleSelectableRows: Story = () => {
interface SelectableData {
column1: string;
column2: string;
Expand Down Expand Up @@ -569,7 +569,46 @@ export const SelectableRows: Story = () => {
},
];
return (
<Table selectableRows columns={columns} data={data} onSelectedRowsChange={console.info} />
<Table selectionMode="multiple" columns={columns} data={data} onSelectedRowsChange={console.info} />
);
};

export const SingleSelectableRows: Story = () => {
interface SelectableData {
column1: string;
column2: string;
column3: number;
}

const columns: TableColumn<SelectableData>[] = [
{
header: 'Column 1',
accessorKey: 'column1',
},
{
header: 'Column 2',
accessorKey: 'column2',
},
{
header: 'Column 3',
accessorKey: 'column3',
},
];

const data: TableData<SelectableData>[] = [
{
column1: 'a',
column2: 'a',
column3: 10,
},
{
column1: 'b',
column2: 'b',
column3: 20,
},
];
return (
<Table selectionMode="single" columns={columns} data={data} onSelectedRowsChange={console.info} />
);
};

Expand Down Expand Up @@ -1236,7 +1275,7 @@ export const WithBackgroundColor: Story = () => {
return (
<ScrollableWrap>
<StyledTableWithBackground
selectableRows
selectionMode="multiple"
stickyHeader
stickyFooter
columns={columns}
Expand Down
Loading