Create column menu #174
Open
Create column menu #174
Conversation
This file contains hidden or 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
- Introduced `ColumnMenu` and `ColumnMenuButton` components for enhanced column management. - Updated `ColumnHeader` to handle column menu visibility and positioning. - Integrated column menu toggle logic in `TableHeader` to manage open state. - Added CSS styles for the column menu button and its hover effects.
…logic - Added `direction`, `sortable`, and `onClick` props to `ColumnMenu` for improved sorting capabilities. - Updated `ColumnMenu` rendering to include sorting buttons and clear sort option based on the current sort direction. - Refactored `ColumnHeader` to pass new props to `ColumnMenu` for better integration.
- Introduced `usePortalContainer` hook and `PortalContainerProvider` to manage portal rendering. - Updated `ColumnMenu` to utilize the new portal container for rendering, enhancing its visibility management. - Refactored `HighTable` to include `PortalContainerProvider`, passing down the container reference. - Enhanced CSS styles for the column menu to improve its appearance and usability.
- Adjusted padding for sortable headers to accommodate sort indicators. - Updated sort indicator styles for better alignment and visibility. - Modified font size for header buttons to ensure consistency in design.
- Updated ColumnHeader to conditionally render the ColumnMenuButton based on the sortable prop. - Enhanced HighTable CSS by adding overflow property for better content management and adjusting margin for sort indicators. - Modified width calculation in the measureWidth function to account for new padding adjustments.
…d usability - Added keyboard navigation support to ColumnMenu and ColumnMenuButton for better accessibility. - Updated ColumnMenu to focus on the menu when it becomes visible. - Refactored ColumnMenuButton to handle keyboard events for activating the menu. - Enhanced HighTable CSS to improve focus styles for column menu buttons.
- Updated ColumnMenuButton to create a synthetic mouse event for keyboard interactions, enhancing accessibility. - Adjusted ColumnHeader to position the column menu based on the button's location, improving usability.
rembrandtreyes
commented
May 18, 2025
src/helpers/width.ts
Outdated
| @@ -13,5 +13,5 @@ export function measureWidth(element: HTMLTableCellElement): number { | |||
| const style = window.getComputedStyle(element) | |||
| const horizontalPadding = parseInt(style.paddingLeft) + parseInt(style.paddingRight) | |||
| // add 1px to avoid rounding errors, since offsetWidth always returns an integer | |||
| return element.offsetWidth - horizontalPadding + 1 | |||
| return element.offsetWidth - horizontalPadding + 24 | |||
There was a problem hiding this comment.
this is to give initial width to account for the sort icon and column menu button
…ality - Added `columnIndex` and `onToggleColumnMenu` props to `ColumnHeader` for better column menu management. - Updated `ColumnMenu` to utilize the new props, allowing for enhanced keyboard navigation and focus management when toggling the column menu.
- Added `buttonRef` prop to `ColumnMenu` and `ColumnMenuButton` for better focus handling during keyboard navigation. - Updated `ColumnHeader` to pass the button reference, ensuring the correct element is focused when the column menu is toggled. - Refactored event handling in `ColumnMenuButton` to utilize the new button reference for synthetic mouse events.
…accessibility - Updated `ColumnHeader` to include `ref` in the dependency array for better event management. - Changed `onToggleColumnMenu` in `ColumnMenu` to be a required prop, ensuring consistent usage. - Enhanced keyboard interaction in `ColumnMenu` by removing optional chaining for `onToggleColumnMenu`. - Added `aria-expanded` and `aria-haspopup` attributes to improve accessibility for screen readers.
- Created unit tests for `ColumnMenu` to verify rendering based on visibility, sorting functionality, and keyboard interactions. - Developed tests for `ColumnMenuButton` to ensure correct ARIA attributes, click handling, and keyboard event responses. - Enhanced `TableHeader` tests to include interactions with the column menu, ensuring proper toggle behavior and sort option visibility.
- Removed optional chaining from the sortable prop in ColumnHeader, ensuring it always passes a boolean value to the ColumnMenu component.
- Included `isColumnMenuOpen` and `onToggleColumnMenu` props in the default props for `ColumnHeader` tests to ensure proper testing of column menu functionality.
severo
reviewed
May 19, 2025
| @@ -47,6 +48,7 @@ interface Props { | |||
| className?: string // additional class names for the component | |||
| columnClassNames?: (string | undefined)[] // list of additional class names for the header and cells of each column. The index in this array corresponds to the column index in data.header | |||
| styled?: boolean // use styled component? (default true) | |||
| containerRef?: RefObject<HTMLDivElement | null> | |||
There was a problem hiding this comment.
We don't want to add this property to Props, because it's used only internally (for HighTableInner, not for HighTable)
So: I think it's better to move
const containerRef = useRef<HTMLDivElement>(null)
to hooks/usePortalContainer.tsx, and use the hook inside HighTableInner to get the ref.
| @@ -0,0 +1,29 @@ | |||
| import { ReactNode, RefObject, createContext, useContext } from 'react' | |||
|
|
|||
There was a problem hiding this comment.
Add
const containerRef = useRef<HTMLDivElement>(null)here
| @@ -18,6 +18,7 @@ interface TableProps { | |||
| export default function TableHeader({ | |||
| header, orderBy, onOrderByChange, dataReady, ariaRowIndex, sortable = true, columnClassNames = [], | |||
| }: TableProps) { | |||
| const [openColumnMenuIndex, setOpenColumnMenuIndex] = useState<number | null>(null) | |||
There was a problem hiding this comment.
I think we have to move this state down to ColumnHeader.tsx (just isMenuOpen). And we must add more implementation details to ensure that only one menu is open at the same time (by setting the rest of the page as "inert"). In particular, when the menu is open, we want:
- to have a different focus order: Tab/Shift Tab should stay in the modal (this means that we must save where the focus was before opening the menu, and focus the element again after closing it)
- same for up/down/left/right arrows: we must stay inside the menu
- to forbid scrolling the rest of the page
- to close the menu when clicking outside it (put a backdrop
<div>above the whole page, that handles the clicks to close the menu)
This current behavior is a bit uncommon and, I think, unexpected:
Screencast.From.2025-05-19.11-02-29.mp4
Some references:
- https://www.w3.org/WAI/ARIA/apg/patterns/dialog-modal/
- https://www.radix-ui.com/primitives/docs/components/dropdown-menu
- we don't want to add any dependency, but these two packages can give ideas for the focus lock and for removing the scroll: https://www.npmjs.com/package/react-focus-lock and https://www.npmjs.com/package/react-remove-scroll
There was a problem hiding this comment.
Giving an update. Still working through the items in this comment. I have the back drop in place and tab/shit-tab stays in the column menu when open. I also have stopped scrolling when column menu is open. Need to get the arrow keys to stay in the modal and save the previous focused element.
There was a problem hiding this comment.
Awesome! It will be a nice feature!
|
I put the main comments in: It's very tricky to get a modal right and accessible, but I think I listed the most important properties we want |
Co-authored-by: Sylvain Lesage <[email protected]>
- Integrated usePortalContainer hook in HighTable for improved container reference management. - Removed redundant containerRef declaration in HighTable, relying on the context provided by usePortalContainer. - Updated PortalContainerProvider to initialize containerRef internally, enhancing encapsulation.
…d accessibility - Removed `isColumnMenuOpen` and `onToggleColumnMenu` props from `ColumnHeader`, replacing them with local state management for menu visibility. - Updated `ColumnMenu` to accept `isOpen` instead of `isVisible`, enhancing clarity in prop naming. - Simplified event handling for toggling the column menu, improving code readability and maintainability. - Ensured proper focus management and accessibility attributes for better user experience.
- Refactored `getSortDirection` function for clarity and moved it outside the component. - Improved accessibility by adding `aria-labelledby` to the menu and ensuring proper focus management. - Updated event handling for overlay clicks and keyboard interactions. - Enhanced styles for the menu overlay and menu items, including hover effects and z-index adjustments for better visibility.
- Introduced `MenuItem` and `Overlay` components for better modularity and readability. - Enhanced scroll lock management using local state to prevent background scrolling when the menu is open. - Improved keyboard interaction handling for better accessibility and user experience. - Updated rendering logic to utilize the new `MenuItem` component for sortable options.
- Introduced focus management to store and restore the previously focused element when the menu opens and closes. - Improved keyboard navigation within the menu, allowing users to cycle through focusable elements using arrow keys and Tab. - Updated styles to provide visual feedback for focused menu items, enhancing accessibility and user experience.
- Updated the toggle logic in ColumnHeader to navigate to the cell only when closing the menu, improving user experience. - Adjusted dependencies in the useEffect hook to include navigateToCell for better state management.
- Added `tabIndex` prop to `ColumnMenuButton` for improved keyboard navigation. - Updated `ColumnHeader` to pass `tabIndex` to `ColumnMenuButton`, ensuring consistent focus management. - Changed background color in HighTable styles for better visual clarity.
…nents for cleaner code.
…ccessibility - Updated ColumnHeader to navigate to the cell before toggling the menu, enhancing user experience. - Improved MenuItem click handling to focus the button when clicked, ensuring better accessibility. - Added logic to prevent key events from propagating when the menu is open, enhancing keyboard navigation.
- Changed outline width to use a CSS variable for better customization. - Added outline offset to enhance focus indication for menu items and buttons.
- Added `aria-expanded` attribute to `ColumnHeader` for improved accessibility. - Updated HighTable styles to enhance focus visibility for buttons when expanded.
…nctionality - Renamed props in ColumnMenu tests for consistency, changing `isVisible` to `isOpen` and `onToggleColumnMenu` to `onToggle`. - Updated accessibility attributes in ColumnMenu tests, replacing `aria-label` with `aria-labelledby`. - Enhanced TableHeader tests by adding `onOrderByChange` and `orderBy` props for better interaction handling. - Improved assertions in TableHeader tests to validate menu accessibility and content based on the new props.
- Adjusted padding-right for cells with sorting enabled to improve layout consistency. - Modified right positioning of the sort arrow to enhance visual alignment.
…code - Removed unused `RefObject` and `MouseEvent` imports from HighTable. - Simplified imports in TableHeader by removing unused `useState` hook.
|
@severo got a little demo of the keyboard nav. I'll push up my changes so you can pull it locally and so I can get some feedback on the keyboard nav. First time tackling keyboard nav with so many tabbable things also introduced with key-nav.mov |
- Replaced manual scroll lock logic in ColumnMenu with the new useScrollLock hook for cleaner code. - Improved focus management by using a constant for focusable element selectors. - Enhanced accessibility by adding aria-label attributes to menu elements.
- Introduced useFocusManagement hook to streamline focus handling in ColumnMenu. - Removed manual focus management logic and replaced it with the new hook for improved accessibility. - Enhanced keyboard navigation by simplifying focus navigation logic for menu items.
…and functionality - Added `menuId` and `aria-label` to `ColumnMenuButton` for better screen reader support. - Updated `handleColumnMenuClick` to accept both `MouseEvent` and `KeyboardEvent`. - Refactored `ColumnMenuButton` to handle keyboard interactions and prevent clicks when disabled. - Improved structure and readability of the `ColumnMenuButton` component.
…tionality - Introduced useColumnMenu hook to manage menu state and positioning, simplifying ColumnHeader logic. - Removed local state management for menu visibility and position, enhancing code clarity. - Updated event handling for ColumnMenuButton to use the new hook's methods for better encapsulation.
…consistency and accuracy - Adjusted expected maxWidth in ColumnHeader test to reflect correct styling. - Updated aria-label in ColumnMenuButton test for improved accessibility. - Modified event type assertions in ColumnMenuButton tests to reflect keyboard interactions. - Changed button role name in TableHeader tests for consistency with updated accessibility standards.
rembrandtreyes
commented
May 30, 2025
| @@ -87,7 +89,7 @@ describe('ColumnHeader', () => { | |||
| // the width is set to undefined, and should then be measured, | |||
| // but the measurement (.offsetWidth) can only run in a browser, | |||
| // so its value is 0 + the 1px offset added to avoid rounding errors | |||
| expect(header.style.maxWidth).toEqual('1px') | |||
| expect(header.style.maxWidth).toEqual('24px') | |||
There was a problem hiding this comment.
Needed to bump this to 24px to now account for the sorting arrows and column menu button
rembrandtreyes
commented
May 30, 2025
Comment on lines
+30
to
+31
| const { isOpen, position, menuId, handleToggle, handleMenuClick } = | ||
| useColumnMenu(columnIndex, ref, navigateToCell) |
There was a problem hiding this comment.
Add useColumnMenu hook to separate concerns
ColumnHeader was handling both column display and menu logic. This hook isolates menu state management, making the component easier to understand and test.
rembrandtreyes
commented
May 30, 2025
| import { useScrollLock } from '../../hooks/useScrollLock' | ||
| import { useFocusManagement } from '../../hooks/useFocusManagement' | ||
|
|
||
| function getSortDirection(direction?: Direction, sortable?: boolean) { |
There was a problem hiding this comment.
Pure utility function moved outside component scope for better performance
rembrandtreyes
commented
May 30, 2025
Comment on lines
+92
to
+93
| useScrollLock(isOpen) | ||
| const { navigateFocus } = useFocusManagement(isOpen, menuRef) |
There was a problem hiding this comment.
Encapsulate complex accessibility logic in dedicated hooks
Scroll prevention and focus trapping have many edge cases. Isolating this logic:
- Makes ColumnMenu easier to understand
- Centralizes accessibility behavior for easier maintenance
- Allows independent testing of focus/scroll logic
…essibility - Updated test descriptions for clarity and accuracy, focusing on column menu integration with table header accessibility. - Improved assertions to validate menu button nesting and labeling within the table header. - Streamlined focus management tests to ensure proper behavior within the table context.
…tion - Introduced onEscape prop to ColumnMenuButton to handle Escape key events. - Updated keyboard interaction logic to call onEscape when the Escape key is pressed. - Enhanced tests for ColumnMenuButton to validate onEscape functionality and ensure proper behavior when disabled.
- Refactored tests to group by functionality, including rendering, sort functionality, keyboard navigation, and overlay interactions. - Added tests for ARIA attributes, position styling, and edge cases such as handling special characters in column names. - Improved keyboard navigation tests to cover various key interactions and ensure proper behavior when sortable. - Updated the ColumnMenu component to conditionally call onClick based on the sortable prop for better interaction handling.
- Simplified test rendering syntax for better clarity. - Updated the handling of empty column names to use single quotes for consistency with other tests.
|
Excellent. Please tell me when you want me to review the PR again! |
…ility - Changed the ColumnMenuButton component from a div to a button element to enhance semantic meaning and accessibility. - Updated related CSS styles to target the button element instead of a div with role="button". - Ensured proper ARIA attributes and keyboard interactions are maintained for the new button structure.
|
@severo it's at a good spot for a review 🙏 thanks! |
…mproved accessibility - Changed the buttonRef in ColumnHeader from HTMLDivElement to HTMLButtonElement for semantic accuracy. - Updated the ref type in ColumnMenuButton tests to reflect the change from div to button, ensuring correct role and tag name assertions.
- Adjusted the minimum width assertion in ColumnHeader tests from 10px to 24px. - Modified ColumnMenuButton tests to check the button's disabled state directly instead of using ARIA attributes, enhancing clarity and accuracy in the tests.
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.
This PR adds a column menu feature to the table headers, allowing users to sort columns through a dropdown menu. The implementation includes:
ColumnMenucomponent with keyboard navigation supportKey features:
The changes maintain the existing table functionality while adding this new feature in a way that's consistent with the codebase's patterns and accessibility requirements.
column-menu.mov