Skip to content

fix cellClassRule type, fix keyboard timeout, small refactoring#123

Merged
dtassone merged 3 commits into
mui:masterfrom
dtassone:cellClassFix
Jul 28, 2020
Merged

fix cellClassRule type, fix keyboard timeout, small refactoring#123
dtassone merged 3 commits into
mui:masterfrom
dtassone:cellClassFix

Conversation

@dtassone

Copy link
Copy Markdown
Contributor

No description provided.

@oliviertassinari oliviertassinari left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I have tried the keyboard behavior without the listener on scrolling and the wait on the request animation frame. It works great with ArrowDown but is junky with ArrowLeft (I guess it's what requestAnimationFrame is for).

In any case, it's much better without the 100ms delay :)

logger.debug(
`Focusing on cell with index ${nextCellIndexes.rowIndex} - ${nextCellIndexes.colIndex} `,
);
nextCell.tabIndex = 0;

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should we change the tabIndex for the next cell at the same time we change it for the previous cell?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

at this stage the previous cell is already changed

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

So, for a short moment, we have all the cells with tabIndex={-1}, none with tabIndex={0}. I was wondering if it's important.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

yes that's correct. I don't think it is 🤔
If you select the first cell and do an END key, your first cell will get removed of the DOM so all tab would be -1 even if we leave at 0 until we reach the other cell.

apiRef.current!.scrollToIndexes(nextCellIndexes);
setTimeout(() => {
const nextCell = getCellElementFromIndexes(root, nextCellIndexes);
apiRef.current!.once(SCROLLING, () => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I'm not sure to follow, why is moving the focus conditional to the grid scrolling?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

because the dom element of the new cell might not be there yet.

setTimeout(() => {
const nextCell = getCellElementFromIndexes(root, nextCellIndexes);
apiRef.current!.once(SCROLLING, () => {
requestAnimationFrame(() => {

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

If we wait for the next animation frame, we might want to can cancel it if we have a new event coming.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm yes! Done :)

@dtassone
dtassone merged commit 22b6f3c into mui:master Jul 28, 2020
dtassone added a commit that referenced this pull request Jul 28, 2020
* Release v0.1.62 (#95)

* v0.1.51

* fix demo app build

* v0.1.52

* v0.1.53

* v0.1.54

* v0.1.55

* v0.1.56

* v0.1.57

* v0.1.58

* v0.1.59

* add missing export

* v0.1.60

* v0.1.61

* hot fix for tsconfig conf

* fix mistypes on the package.json

* fix empty space with prettier

* v0.1.62

* fix lerna.json whitespace

* fix resize issue (#96)

* Bump eslint-config-airbnb-typescript from 8.0.2 to 9.0.0 (#112)

* Pagination refactoring (#100)

* implemented server pagination

* rename pagination options, doc, prettier

* fix lint

* Apply suggestions from code review

Co-authored-by: Olivier Tassinari <[email protected]>

* change casing value in featureMode and remove apiref check

* added missing dep in useEffect

* fix broken suggestion code

* fix doc comments

* Apply suggestions from code review

Co-authored-by: Olivier Tassinari <[email protected]>

* Fix FeatureMode enum and remove useCallback in demo

* fix import order

* added namespace to function call to remove import

* remove useCallback and add react. to mdx code

* add concurrency in server pagination demo

* fix feature mode export

Co-authored-by: Olivier Tassinari <[email protected]>

* [core] Use the classnames helper (#105)

* [core] Batch small changes (#104)

* [docs] Improve the JSDoc (#106)

* rename events and add onHover event (#103)

* rename events and added onHover event

* rename events and added onHover event

* fix onPageChange rename and merge

* Js doc improvements

Co-authored-by: Olivier Tassinari <[email protected]>

* added hover stories

* small fix and cleanup

* revert rename, back to onRowSelected

* refactor hover and click handlers

* refactor cellParams

* prettier

Co-authored-by: Olivier Tassinari <[email protected]>

* [test] Add yarn deduplicate step (#107)

* [DataGrid] Set default logLevel to warn (#117)

* override default logLevel to warn

* rerun CI

Co-authored-by: Olivier Tassinari <[email protected]>

* [test] Lint JSON (#108)

* Merging release back to master (#120)

* v0.1.51

* fix demo app build

* v0.1.52

* v0.1.53

* v0.1.54

* v0.1.55

* v0.1.56

* v0.1.57

* v0.1.58

* v0.1.59

* add missing export

* v0.1.60

* v0.1.61

* hot fix for tsconfig conf

* fix mistypes on the package.json

* fix empty space with prettier

* v0.1.62

* fix lerna.json whitespace

* v0.1.63

* v0.1.64

* prettier lerna.json due to lerna

* fix cellClassRule type, fix keyboard timeout, small refactoring (#123)

* fix cellClassRule type, fix keyboard timeout, small refactoring

* prettier fix

* add cancel RAF for focus if new event coming

* added keyboard doc (#125)

* added keyboard doc

* Apply suggestions from code review

Co-authored-by: Olivier Tassinari <[email protected]>

Co-authored-by: Olivier Tassinari <[email protected]>

* [DataGrid] Fix selection issue (#126)

* fix selection issue

* fix selection issue test

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Co-authored-by: Olivier Tassinari <[email protected]>
@zannager zannager added the scope: data grid Changes related to the data grid. label Jan 17, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

scope: data grid Changes related to the data grid.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants