-
Notifications
You must be signed in to change notification settings - Fork 127
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
fix(DataGrid): add checkbox support #5380
fix(DataGrid): add checkbox support #5380
Conversation
✅ Deploy Preview for carbon-for-ibm-products ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@@ -485,7 +506,7 @@ export const InlineEditCell = ({ | |||
config?.validator?.(cellValue), | |||
})} | |||
> | |||
{!inEditMode && ( | |||
{!inEditMode && type !== 'checkbox' && ( |
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.
Because on UI checkbox
will always be visible, so no need to do any enter to edit mode here
Hi @kennylam and @sangeethababu9223 can you start the review process on this PR please? Thanks! |
Hey @AustinGitHub, there are some spacing issues in the cell with the checkbox, also the active cell is not able to navigate to the cells with the checkbox in it like it's able to in all of the other cells. |
"there are some spacing issues in the cell with the checkbox" How should the spacing be like? "also the active cell is not able to navigate to the cells with the checkbox in it like it's able to in all of the other cells." It shouldn't need to. The cell for the checkbox isn't like something that needs to be double clicked to activate and go into for the checkbox. It is not hidden behind the inline edit component. Otherwise we would need to double click for checkbox to work |
The spacing should match the screenshot in #5302. My expectation would be that moving the active cell would focus the checkbox, otherwise you lose context for the location of the active cell. I think this could probably use some design input as well. |
Ok, I will fix the positioning |
@matthewgallo I think the edit session is fine. I see these props |
Yes, but it visually disappears from what I am seeing. I think it could be confusing to be navigating through the datagrid and suddenly the active cell disappears when you move to a checkbox cell, giving the checkbox focus could fix this. @jjennings7 do you have any thoughts on this interaction? |
I am not seeing this, can you provide video? Thanks! Screen.Recording.2024-06-04.at.8.40.29.AM.mov |
Sure, this is with navigating via keyboard. |
Thanks, will take a look at this use case |
fix: focus for the cell
Screen.Recording.2024-06-04.at.9.04.09.AM.movThis is fixed in latest commit, let me resolve conflicts Edit: Fixing some issues due to the conflicts, notice it isn't working anymore |
It should be good now on the keyboard behavior I went through and hit enter and escape to capture the behavior Screen.Recording.2024-06-04.at.9.49.41.AM.mov |
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.
@AustinGitHub ,
There seems to be some mismatch with focus behaviour in PR preview story and the Production one.
Using Up
/ Down
/ Right
/ Left
keys on the keyboard works as expected, but the behaviour with the tab
key is not the same as the one in Production
ok, I will check on tab behavior, thanks! |
Hi @sangeethababu9223 I see production doesn't work properly for Tab either. I think that needs to be taken up in a separate issue Screen.Recording.2024-06-10.at.8.19.22.AM.mov |
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.
Hi @AustinGitHub,
The focus behaviour in the production is as per w3 standards.
Please refer the link for details.
As per the above reference navigation inside the grid is done by up
/down
/left
/right
keys.
Hi, Thanks! |
Hi @AustinGitHub, Preview: Screen.Recording.-.Preview.movProduction: Screen.Recording.-.Production.movBoth preview and production works as expected while using |
ok, I will check |
So is this the expected behavior then? Screen.Recording.2024-06-11.at.10.27.57.PM.movThough, I am still confused on why the behavior is like this and why design is asking for the tab to be like that as I would figure the tab would want to go into the next cell rather than this weird column selection it is doing |
Hi, any update on this? Thanks! |
Conflicts are fixed |
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.
LGTM
Thanks! @sangeethababu9223 can you remove the conflicts tag and add the pending one more review tag please? Thanks! |
b1761a0
Closes #5302
Adds checkbox support
What did you change?
Adds checkbox support as an optional inline edit type.
Reason I made the PR is because NextGen DataStage needs this and doesn't seem there is much movement to get this implemented. Thanks!
How did you test and verify your work?
Verification
Screen.Recording.2024-05-30.at.5.11.01.PM.mov
I also validated that the data does get updated and set properly
see it
isActive
is set to true since it as checked