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

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

Merged
merged 3 commits into from
Jul 28, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
678c1fc
fix cellClassRule type, fix keyboard timeout, small refactoring
dtassone Jul 28, 2020
e47ffe8
prettier fix
dtassone Jul 28, 2020
cb134b0
add cancel RAF for focus if new event coming
dtassone Jul 28, 2020
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
2 changes: 1 addition & 1 deletion packages/grid/x-grid-modules/src/components/row-cells.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import { buildCellParams } from '../utils/paramsUtils';

function applyCssClassRules(cellClassRules: CellClassRules, params: CellClassParams) {
return Object.entries(cellClassRules).reduce((appliedCss, entry) => {
const shouldApplyCss: boolean = entry[1](params);
const shouldApplyCss: boolean = isFunction(entry[1]) ? entry[1](params) : entry[1];
appliedCss += shouldApplyCss ? `${entry[0]} ` : '';
return appliedCss;
}, '');
Expand Down
13 changes: 11 additions & 2 deletions packages/grid/x-grid-modules/src/hooks/features/usePagination.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,12 +177,21 @@ export const usePagination = (
logger.info(`Options or rows change, recalculating pageCount and rowCount`);
const newPageCount = getPageCount(state.pageSize, rowCount);

updateState({ pageCount: newPageCount, rowCount: rows.length });
updateState({ pageCount: newPageCount, rowCount });
if (state.page > newPageCount) {
setPage(newPageCount);
}
}
}, [rows.length, logger, updateState, state.rowCount, state.pageSize, setPage, state.page]);
}, [
rows.length,
options.rowCount,
logger,
updateState,
state.rowCount,
state.pageSize,
setPage,
state.page,
]);

React.useEffect(() => {
if (
Expand Down
8 changes: 4 additions & 4 deletions packages/grid/x-grid-modules/src/hooks/root/useApi.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ export function useApi(
}
return eventParams;
},
[emitEvent, apiRef],
[apiRef],
);

const onClickHandler = React.useCallback(
Expand All @@ -158,7 +158,7 @@ export function useApi(
emitEvent(COLUMN_HEADER_CLICK, eventParams.header);
}
},
[emitEvent, apiRef],
[emitEvent, getEventParams],
);

const onHoverHandler = React.useCallback(
Expand All @@ -179,7 +179,7 @@ export function useApi(
emitEvent(COLUMN_HEADER_HOVER, eventParams.header);
}
},
[emitEvent, apiRef],
[emitEvent, getEventParams],
);

const registerEvent = React.useCallback(
Expand Down Expand Up @@ -237,7 +237,7 @@ export function useApi(
}

return undefined;
}, [gridRootRef, isApiInitialised, getHandler, logger, onClickHandler, apiRef]);
}, [gridRootRef, isApiInitialised, getHandler, logger, onClickHandler, onHoverHandler, apiRef]);

useApiEventHandler(apiRef, COL_RESIZE_START, handleResizeStart);
useApiEventHandler(apiRef, COL_RESIZE_STOP, handleResizeStop);
Expand Down
30 changes: 21 additions & 9 deletions packages/grid/x-grid-modules/src/hooks/root/useKeyboard.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {
KEYDOWN_EVENT,
KEYUP_EVENT,
MULTIPLE_KEY_PRESS_CHANGED,
SCROLLING,
} from '../../constants/eventsConstants';
import {
findGridRootFromCurrent,
Expand Down Expand Up @@ -45,6 +46,7 @@ const getNextCellIndexes = (code: string, indexes: CellIndexCoordinates) => {
export const useKeyboard = (options: GridOptions, initialised: boolean, apiRef: ApiRef): void => {
const logger = useLogger('useKeyboard');
const isMultipleKeyPressed = React.useRef(false);
const rafFocusOnCellRef = React.useRef(0);

const onMultipleKeyChange = React.useCallback(
(isPressed: boolean) => {
Expand Down Expand Up @@ -106,19 +108,29 @@ export const useKeyboard = (options: GridOptions, initialised: boolean, apiRef:
nextCellIndexes.colIndex =
nextCellIndexes.colIndex >= colCount ? colCount - 1 : nextCellIndexes.colIndex;

apiRef.current!.scrollToIndexes(nextCellIndexes);
setTimeout(() => {
const nextCell = getCellElementFromIndexes(root, nextCellIndexes);
if (rafFocusOnCellRef.current) {
cancelAnimationFrame(rafFocusOnCellRef.current);
}

if (nextCell) {
nextCell.tabIndex = 0;
(nextCell as HTMLDivElement).focus();
}
}, 100);
apiRef.current!.once(SCROLLING, () => {
Copy link
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
Member 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.

rafFocusOnCellRef.current = requestAnimationFrame(() => {
const nextCell = getCellElementFromIndexes(root, nextCellIndexes);

if (nextCell) {
logger.debug(
`Focusing on cell with index ${nextCellIndexes.rowIndex} - ${nextCellIndexes.colIndex} `,
);
nextCell.tabIndex = 0;
Copy link
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
Member 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
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
Member 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.

(nextCell as HTMLDivElement).focus();
}
});
});

apiRef.current!.scrollToIndexes(nextCellIndexes);

return nextCellIndexes;
},
[apiRef, options.pagination, options.pageSize],
[apiRef, options.pagination, options.pageSize, logger],
);

const selectActiveRow = React.useCallback(() => {
Expand Down
4 changes: 3 additions & 1 deletion packages/grid/x-grid-modules/src/models/cellClass.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,4 +15,6 @@ export type CellClassNamePropType = string | string[] | CellClassFn;
/**
* An object representing the cell class rules.
*/
export type CellClassRules = { [cssClass: string]: (params: CellClassParams) => boolean };
export type CellClassRules = {
[cssClass: string]: ((params: CellClassParams) => boolean) | boolean;
};
Binary file modified ...-test-ts-snapshot-test-story-x-grid-tests-columns-update-columns-btn-2-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
Binary file modified ...-ts-snapshot-test-story-x-grid-tests-styling-column-cell-class-rules-1-snap.png
View file
Open in desktop
Loading
Sorry, something went wrong. Reload?
Sorry, we cannot display this file.
Sorry, this file is invalid so it cannot be displayed.
1 change: 1 addition & 0 deletions packages/storybook/src/stories/grid-style.stories.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,7 @@ export const ColumnCellClassRules = () => {
cols[4].cellClassRules = {
common: (params) => params.data.lastName === 'Smith',
unknown: (params) => !params.data.lastName,
border: true,
};

return (
Expand Down
3 changes: 3 additions & 0 deletions packages/storybook/src/style/grid-stories.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
.unknown {
background: coral;
}
.border {
border: 1px solid orangered;
}
#root {
top:0;
left: 0;
Expand Down