Accessibility, keyboard nav, major bug fixes #5

dtassone · 2020-05-21T16:42:45Z

Added keyboard navigation/selection, added aria attributes, fix major issue with virtualisation

oliviertassinari · 2020-05-22T13:59:58Z

packages/grid/src/components/cell.tsx

@@ -26,10 +41,14 @@ export const Cell: React.FC<GridCellProps> = React.memo(
    return (
      <div
        className={cssClasses}
-        role={field + ' cell'}
+        role={'cell'}


Suggested change

role={'cell'}

role="cell"

oliviertassinari · 2020-05-24T21:25:23Z

packages/demo-app/src/app/demos/grid/real-data-grid.demo.tsx

@@ -31,8 +31,17 @@ export const CommodityGridDemo: React.FC<{}> = props => {
    setCols(gridColumns);
    setLoading(true);

-    loadFile(`./static-data/${type}-${size}.json`).then(
+    loadFile(`./static-data/${type}-1000.json`).then(


⚠️ with the clean-up. I would recommend the following: https://github.com/mui-org/material-ui/blob/90babbd13d8dc624ff23453a4e47a311e2758aa6/docs/src/pages/components/autocomplete/Asynchronous.tsx#L23-L43. Notice the active variable that avoids calling setState on an unmounted component (if the request resolves after the unmount).

oliviertassinari · 2020-05-24T21:27:28Z

packages/grid-data-generator/src/commodities.columns.tsx

+  StatusRenderer,
+} from './renderer';
+
+const totalPriceFormatter = ({ value }) => `$ ${Number(value).toLocaleString()}`;


Are you sure about the space between $ and the figures?

console.log(new Intl.NumberFormat("en-US", {style: "currency", currency: "USD"}).format(121)); // $121.00.

oliviertassinari · 2020-05-24T21:29:09Z

packages/grid-data-generator/src/commodities.columns.tsx

+} from './renderer';
+
+const totalPriceFormatter = ({ value }) => `$ ${Number(value).toLocaleString()}`;
+const pnlFormatter = params =>


We have applied the following coding convention so far: top-level scope => named function, nested-level scope => arrow function. In this context, it would turn into

function pnlFormatter(params) {

if we want to keep following this convention.

oliviertassinari · 2020-05-24T21:33:50Z

packages/grid-data-generator/src/renderer/rating-renderer.tsx

+import React from 'react';
+
+export function RatingRenderer(params: CellParams) {
+  return <Rating name={params.data['id'].toString()} value={Number(params.value)} readOnly />;


Maybe?

-return <Rating name={params.data['id'].toString()} value={Number(params.value)} readOnly />; +return <Rating name={params.data.id.toString()} value={Number(params.value)} readOnly />;

oliviertassinari · 2020-05-24T21:40:16Z

packages/grid/src/components/column-header-sort-icon.tsx

@@ -19,10 +20,16 @@ export const ColumnHeaderSortIcon: React.FC<ColumnHeaderSortIconProps> = React.m
      <span className={'sort-icon'}>
        {index != null && (
          <Badge badgeContent={index} color="default">
-            {icons[direction]}
+            <IconButton aria-label="Sort" size="small">


We prefer not to capitalize on the label, as far as I know, it makes no difference during pronunciation.

<IconButton aria-label="sort" size="small">

oliviertassinari · 2020-05-24T21:40:51Z

packages/grid/src/components/column-header-title.tsx

@@ -7,7 +7,7 @@ const ColumnHeaderInnerTitle = React.forwardRef<HTMLDivElement, any>((props, ref
  const { label, className, ...rest } = props;

  return (
-    <div ref={ref} className={'title ' + className} {...rest} role={'column-header-title'}>
+    <div ref={ref} className={'title ' + className} {...rest} role={'column-header-title'} aria-label={label}>


oliviertassinari · 2020-05-24T21:42:47Z

packages/grid/src/components/column-headers.tsx

+      />
+    ));
+
+    return <>{items}</>;


I would expect it to work without a Fragment.

return items;

oliviertassinari · 2020-05-24T21:53:58Z

packages/grid/src/hooks/root/useKeyboard.ts

+    nextCellIndexes.colIndex = nextCellIndexes.colIndex >= colCount ? colCount - 1 : nextCellIndexes.colIndex;
+
+    apiRef.current!.scrollToIndexes(nextCellIndexes);
+    setTimeout(() => {


It seems we miss a clean-up.

oliviertassinari · 2020-05-24T21:59:25Z

packages/grid/src/hooks/features/useColumnResize.tsx


+  //This a hack due to the limitation of react as I cannot put columnsRef in the dependency array of the effect adding the Event listener


I would love to have a chat about how we can improve the approach

oliviertassinari · 2020-05-24T22:05:13Z

Regarding react-hooks/exhaustive-deps I think that it would be valuable to turn the rule into an error. It should almost never be necessary to ignore the rule. From experience, anytime we ignore this warning, a developer will raise a bug, later down the road.

oliviertassinari · 2020-05-24T22:31:39Z

Is it expected that the Grid container can receive the focus?
keyboard control can be brittle, adding a test case per command could help.
I have just tried Control + Home, pretty cool 👌.
Should we send this codesandbox to Josh so he can try it out https://mf8le.csb.app/?
I believe the behavior on the header is not correct, in respect to how the sorting can be triggered. However, happy to revisit later.

* Make chart docs great again * [Charts] Fix Popper complaining about not being anchored to a DOM element * [Charts] Fix infinite render loop in Line

dtassone added 9 commits May 18, 2020 16:42

WIP fix keyboard events and small refactoring

56d96eb

WIP Keyboard navigation working more or less

d4f05ad

WIP fix rendering engine

bb0fe13

Fix resize columns rendering of items

9c96e5b

WIP Keyboard navigation working well

02228e8

added keyboard shortcuts and accessibility

8ba6891

fix perf issue by adding virtualisation on column header row

bbbb517

fix state race condition, fix checkbox header state on new rows, cleanup

abcfa65

removed 100k files, improved perf

0c55f44

oliviertassinari mentioned this pull request May 22, 2020

Discrepancy with main repository #6

Closed

12 tasks

dtassone added 4 commits May 22, 2020 16:47

restored border right options, and fix small issue with keyboard

a64bd85

improved perf on cols, small refactoring

84faac0

commented row highlight for virtualisation

58b70ff

Fix lint

0ad5f5d

dtassone merged commit 0ad5f5d into master May 22, 2020

oliviertassinari deleted the accessibility branch May 24, 2020 21:50

oliviertassinari reviewed May 24, 2020

View reviewed changes

oliviertassinari mentioned this pull request May 25, 2020

[Calendar] Remove promise based loading in favor of loading prop mui/material-ui-pickers#1829

Merged

dtassone restored the accessibility branch May 26, 2020 10:05

oliviertassinari deleted the accessibility branch May 27, 2020 11:03

dtassone restored the accessibility branch May 27, 2020 17:25

dtassone deleted the accessibility branch July 21, 2020 14:33

oliviertassinari added the component: data grid This is the name of the generic UI component, not the React module! label Jan 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Accessibility, keyboard nav, major bug fixes #5

Accessibility, keyboard nav, major bug fixes #5

dtassone commented May 21, 2020

oliviertassinari May 22, 2020

oliviertassinari May 24, 2020

oliviertassinari May 24, 2020

oliviertassinari May 24, 2020

oliviertassinari May 24, 2020

oliviertassinari May 24, 2020

oliviertassinari May 24, 2020

oliviertassinari May 24, 2020

oliviertassinari May 24, 2020

oliviertassinari May 24, 2020

oliviertassinari commented May 24, 2020 •

edited

Loading

oliviertassinari commented May 24, 2020


		//This a hack due to the limitation of react as I cannot put columnsRef in the dependency array of the effect adding the Event listener

Accessibility, keyboard nav, major bug fixes #5

Accessibility, keyboard nav, major bug fixes #5

Conversation

dtassone commented May 21, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliviertassinari commented May 24, 2020 • edited Loading

oliviertassinari commented May 24, 2020

oliviertassinari commented May 24, 2020 •

edited

Loading