Accessibility, keyboard nav, major bug fixes#5
Conversation
| <div | ||
| className={cssClasses} | ||
| role={field + ' cell'} | ||
| role={'cell'} |
There was a problem hiding this comment.
| role={'cell'} | |
| role="cell" |
| setLoading(true); | ||
|
|
||
| loadFile(`./static-data/${type}-${size}.json`).then( | ||
| loadFile(`./static-data/${type}-1000.json`).then( |
There was a problem hiding this comment.
active variable that avoids calling setState on an unmounted component (if the request resolves after the unmount).
| StatusRenderer, | ||
| } from './renderer'; | ||
|
|
||
| const totalPriceFormatter = ({ value }) => `$ ${Number(value).toLocaleString()}`; |
There was a problem hiding this comment.
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.| } from './renderer'; | ||
|
|
||
| const totalPriceFormatter = ({ value }) => `$ ${Number(value).toLocaleString()}`; | ||
| const pnlFormatter = params => |
There was a problem hiding this comment.
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.
| import React from 'react'; | ||
|
|
||
| export function RatingRenderer(params: CellParams) { | ||
| return <Rating name={params.data['id'].toString()} value={Number(params.value)} readOnly />; |
There was a problem hiding this comment.
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 />;| {index != null && ( | ||
| <Badge badgeContent={index} color="default"> | ||
| {icons[direction]} | ||
| <IconButton aria-label="Sort" size="small"> |
There was a problem hiding this comment.
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">|
|
||
| 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}> |
There was a problem hiding this comment.
<div ref={ref} className={'title ' + className} role={'column-header-title'} aria-label={label} {...rest}>| /> | ||
| )); | ||
|
|
||
| return <>{items}</>; |
There was a problem hiding this comment.
I would expect it to work without a Fragment.
return items;| nextCellIndexes.colIndex = nextCellIndexes.colIndex >= colCount ? colCount - 1 : nextCellIndexes.colIndex; | ||
|
|
||
| apiRef.current!.scrollToIndexes(nextCellIndexes); | ||
| setTimeout(() => { |
There was a problem hiding this comment.
It seems we miss a clean-up.
| }; | ||
| }, []); | ||
|
|
||
| //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 |
There was a problem hiding this comment.
I would love to have a chat about how we can improve the approach
|
Regarding |
|
* Make chart docs great again * [Charts] Fix Popper complaining about not being anchored to a DOM element * [Charts] Fix infinite render loop in Line
2 participants
Added keyboard navigation/selection, added aria attributes, fix major issue with virtualisation