-
Notifications
You must be signed in to change notification settings - Fork 71
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
Table - Checkbox cell type #2583
Table - Checkbox cell type #2583
Conversation
✅ Deploy Preview for moduswebcomponents ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
"false": { | ||
checked: false, | ||
} | ||
} |
Check notice
Code scanning / CodeQL
Semicolon insertion Note
the enclosing script
...DefaultColumns.slice(DefaultColumns.length - 1) | ||
], | ||
data: makeData(7) | ||
} |
Check notice
Code scanning / CodeQL
Semicolon insertion Note
the enclosing script
@coliff @egunther39 @enowak1031
I could see having a boolean type column and/or a checkbox editor type. I imagine we will need the Modus Design System to weigh in on this submission as well. @Kai-Richardson If you haven't already, can you make sure there is an Aha! submission for this so a review can be made? We are currently investigating custom html cells for both view and edit. If we can figure it out, that would solve for this case but it would be good to know how the Modus Design System wasn't to work with boolean values in tables. |
@Kai-Richardson Did you have an opportunity to create the Aha submission? |
@egunther39 Can you weigh in on the styling here. I don't see a "view" mode for this new checkbox column type. So the column is always an editable checkbox. Do we have an idea on how it should look when not in edit mode. Keep in mind this is not the row selection checkboxes, but a new column type for boolean fields. |
@cjwinsor - It's a bit hard to follow here. Is there a visual somewhere that illustrates the issue? |
The above shows the submissions checkbox column. Its only every a checkbox, with no "read only" mode such as having a modus check icon / nothing when not being edited. The UX consideration is whether we need a "read only" mode to the column as we don't currently have cells always in edit mode. This PR introduces a column with |
Description
In https://github.com/Trimble-Construction/cascade-web, we have a table layout and we want to render boolean values within a table. This table then gets modified by the user, and the table state gets 'applied' to the application via a button click.
Essentially, there's no good way to render boolean values currently, besides turning it into text and potentially rendering it as a badge, plaintext, or similar.
This feature does differ from the existing 'normal table view' vs 'edit table view' paradigm where the data is view-only for the table until entering edit mode. I could make the checkboxes default to
disabled: false
. I would be interested in the maintainers thoughts on this.The reason I didn't just add this as an edit mode was because the display was lacking as well. A possible alternative implementation would be an editable True/False badge, but I thought that was somewhat clunky and a pain for people editing values via keyboard navigation (a very common usecase).
Most of the inspiration for the code in this PR came from the implementation of Table Badge rendering in #1912.
I also created a typescript type for the properties of
ModusCheckbox
, like was done forModusBadge
.Future Improvements:
KEYBOARD_SPACE/KEYBOARD_ENTER
while focusing the table cell toggle the inner checkbox. I ran into some difficulties in implementing this properly.This could also be done with a Switch instead in the cell - can add those if necessary re: styleguide
However, I think Checkboxes are correct because this does not conform the style of Switches, mainly
No current issue open for this, can make one if required.
Relevant: #1574
IMPORTANT: I'm unsure why
checkboxClick
's type is changing (despite the type only being aliased), will need to fix before merging as I think that constitutes a breaking change. To me this almost reads as a bug due to our extremely out-of-date Stencil version.Type of change
How Has This Been Tested?
I ran this by creating a table layout and passing the relevant data within the index.html set up for use with Stencil. There's currently no e2e testing for the clickability of the table component itself.
Storybook works well, sort works.
Checklist