-
Notifications
You must be signed in to change notification settings - Fork 38
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 markup in Table
's column manager
#2135
Merged
r100-stack
merged 27 commits into
main
from
r/remove-aria-hidden-from-action-column-checkbox
Jul 10, 2024
Merged
Fix markup in Table
's column manager
#2135
r100-stack
merged 27 commits into
main
from
r/remove-aria-hidden-from-action-column-checkbox
Jul 10, 2024
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
r100-stack
changed the title
Fix markup in Table's ActionColumn (column manager)
Fix markup in Jul 9, 2024
Table
's column manager
mayank99
reviewed
Jul 9, 2024
…ove-aria-hidden-from-action-column-checkbox
r100-stack
requested review from
mayank99 and
Ben-Pusey-Bentley
and removed request for
a team
July 9, 2024 20:45
An error occurred while trying to automatically change base from
mayank/fieldset-base
to
main
July 10, 2024 00:12
…-column-checkbox' into r/remove-aria-hidden-from-action-column-checkbox
This comment was marked as resolved.
This comment was marked as resolved.
Ben-Pusey-Bentley
approved these changes
Jul 10, 2024
mayank99
reviewed
Jul 10, 2024
mayank99
previously approved these changes
Jul 10, 2024
mayank99
approved these changes
Jul 10, 2024
Merged
2 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Changes
Fixes #2134. Upon investigating that issue, I noticed that there were a few issues in the markup. E.g.:
<input>
(checkbox) as a descendant of a<button>
<div>
not allowed as a child of<button>
in this contextMenuItem markup in the column manager
Furthermore, @mayank99 added that even if each menu item was a div, no interactable elements are allowed in
role="menuitem"
.As a result, we decided to replace the
DropdownMenu
+MenuItem startIcon={<Checkbox>}
with aPopover
+ floating<fieldset>
with a<Checkbox label={…}>
for each column. Using aPopover
also makes it easy to expand on column manager functionality to address #783.Created a new
iui-table-column-manager
class mainly for padding and max-block-size styles. Likeiui-menu
, this also shows a maximum of 8.5 items (hardcoded without any calculation (#2135 (comment))). The0.5
part is to show that there are more items in the list.Unrelated: Added
@default
to the JSDocs and a default value to the deconstructed object for the Surface'sisPadded
prop.Testing
Docs
Added changesets
After PR TODOs
<SomeComponent as={Flex}>
not override the rendered element" (#2135 (comment)). Related: #2152 (comment)