-
Notifications
You must be signed in to change notification settings - Fork 34
feat(#2609): add data-grid component #2993
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.
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
base: alpha
Are you sure you want to change the base?
Conversation
2c8e4df
to
7fce4c9
Compare
f6acf37
to
29dbfa2
Compare
29dbfa2
to
a4c76ee
Compare
mr={mr} | ||
mb={mb} | ||
ml={ml} | ||
{...dataGridProps} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note for code reviewer and @chrisolsen
Without adding data-xx
attributes to react component (angular works very well), for example:
<GoabContainer key={user.idNumber} mt="l" data-gridrow>
The react wrapper will render as the below, with goa-container
without custom attributes.
As a result, I end up adding dataGridProps
, which allows data-grid
prefix only, and then assign to react components. I only add for components that makes sense, except some such as Progress spinner
or layout column
44eb001
to
b445d31
Compare
} | ||
return focusableNode; | ||
return shouldFocus(node); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to code reviewer:
I moved the logic to shouldFocus
atutils
since DataGrid needs it as well.
c5f9c8c
to
f30db1b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vanessatran-ddi I reviewed the data grid through the demo you shared here: https://wonderful-chaja-1b63f2.netlify.app/data-grid-examples
Awesome work on this so far. Below are some minor issues I found:
- There is something odd happening for the focus when tabbing to the container for the first time. It looks like there is a second blue focus in addition to the standard yellow border.
https://jam.dev/c/988b51b9-580e-435a-9e30-45260d534443
- It might be an issue with my original implementation of this menu on the icon button in the table, but it's not accessible via keyboard. You can open it, and then focus does not move to it.
https://jam.dev/c/5922bf6e-0370-4c2f-a65d-9a95376abfbb
- Again, might be an issue with how I implemented it originally, but the Link to the detail page from the Name is not able to be selected through the focus moving through the data grid
https://jam.dev/c/6f25727d-8c23-49ac-93ff-f96c313b3e94
Also, when I tab when inside the table, it should go between the interactive elements. It does not, it skips everything in the table other than the sortable headers.
https://jam.dev/c/88e6ba53-603b-4bd7-a212-b35415afad07
- It's hard to tell exactly because the dropdown is the last item in the container, but in the data grid when you press down-arrow it moves to the next card. But when you press down on the dropdown it opens up the dropdown menu and goes to the option. Should this open the menu or move past it?
https://jam.dev/c/7acce053-243e-4430-b579-63b9c41ac700
- Regarding the keyboard navigation on the data-grid when using a table: When the user gets to the edge of the table and clicks to the edge again, it should not move.
Data-grid:
- When focus is on the last cell (on an edge) focus does not move.
- Ours currently moves to the next row, should not move.

Layout-grid:
- When focus is on the last cell (on an edge) focus moves to the first cell in the next row in that direction.
- Our works correctly, moving to the first cell in that direction.

3f0c995
to
d9cf9a7
Compare
Hi @twjeffery I uploaded the react version on https://stately-puffpuff-b4791f.netlify.app/(I used the simpler version to test here instead of the workspace demo, after this is on alpha and we can integrate vs the code easier using React, I tried my best to solve some logic but it was taking me time, so I stopped)
Vanessa: I believe this is fixed on the new preview link above. ✅
Vanessa: Yes, it isn't an issue with this PR. The MenuButton from Chris will handle the arrow key well if Popover is opened (this is a part of a reason why I don't add logic to the angular workspace demo), let's wait till @chrisolsen PR merged to alpha, and we can integrate later.✅
Vanessa: Yes on the implementation we are using The preview link above, I am using this: Note: The focus border is blue now, is same vs our Link, so you don't see the yellow border, unless this ticket wants to cover focus css for ![]()
Vanessa: I fixed this, let me know if it happens again ✅
Vanessa: According to the daily standup meeting, we will accept that dropdown will override the arrow key keyboard behaviour, until user press
Vanessa: Yes I fixed it, now if you focus on Open, use click to click on the cell, Open still remains focused. ![]()
Vanessa: Since we are using this https://www.w3.org/WAI/ARIA/apg/patterns/grid/examples/data-grids/, I will not move the focus when we already end of the row (or first cell of the row)✅ |
371b40e
to
7489c6e
Compare
7489c6e
to
34194eb
Compare
Before (the change)
After (the change)
Make sure that you've checked the boxes below before you submit the PR
Steps needed to test
React code:
Simplest test:
data-grid-link.mov
For keyboardNav="layout", when users press Arrow Right at the end of a row, the focus wraps to the first cell of the next row. When users press Arrow Left at the beginning of a row, the focus wraps to the last cellof the previous row.
Table:
data-grid-table.mov
For keyboardNav="table", when users press Arrow Right at the end of a row, the focus stays there. When users press Arrow Left at the beginning of a row, the focus stays there.
Container:
For this example, the cell order is assigned, arrow left and right follow the cell order.
container-data-grid.mov