Focus visible style refinements #1577
Conversation
|
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
apps/webapp/app/components/primitives/Table.tsxOops! Something went wrong! :( ESLint: 8.45.0 ESLint couldn't find the config "custom" to extend from. Please check that the name of the config is correct. The config "custom" was referenced from the config file in "/.eslintrc.js". If you still have problems, please stop by https://eslint.org/chat/help to chat with the team. WalkthroughThis pull request introduces a series of UI and styling enhancements across multiple components in the web application. The changes focus on improving visual consistency, accessibility, and component flexibility. Key modifications include updates to Tailwind CSS configuration, refinement of table and input components, and the introduction of new color palettes and focus styles. The changes are primarily cosmetic and structural, maintaining the existing functional logic while enhancing the overall user interface experience. Changes
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@trigger.dev/react-hooks
@trigger.dev/rsc
@trigger.dev/build
trigger.dev
@trigger.dev/sdk
@trigger.dev/core
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webapp/app/components/primitives/Table.tsx (3)
113-117: Ensure that disabled rows remain truly non-interactive.You set tabIndex to -1 for disabled rows and 0 otherwise, but you’re also setting onClick and onKeyDown. If “disabled” truly means non-interactive, consider removing these event handlers or preventing them from doing anything when disabled is true.
onClick={disabled ? undefined : handleInteraction} onKeyDown={disabled ? undefined : handleKeyDown}
163-163: Revisit negative tabIndex on header cells.Similar to line 47, confirm that negative tabIndex is the intended way to skip focus on TableHeaderCell. This can be acceptable, but it’s worth verifying against your accessibility goals.
192-192: Row hover & focus styling approach.The new focus and hover style classes align with the “Focus visible” changes described in the PR. Nice consistency. Beware potential overlap between :hover, :focus, and :focus-visible states for color or background clashes on older browsers.
apps/webapp/tailwind.config.js (1)
Line range hint
308-316: Well-implemented focus utility using :focus-visibleThe
.focus-customutility correctly uses:focus-visibleinstead of:focus, which is a best practice as it:
- Only shows focus indicators for keyboard navigation
- Hides focus indicators when clicking/tapping
- Provides consistent focus styles across components
Consider documenting this utility in your component library or style guide to encourage consistent usage across the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/webapp/app/components/primitives/Input.tsx(2 hunks)apps/webapp/app/components/primitives/SimpleSelect.tsx(1 hunks)apps/webapp/app/components/primitives/Table.tsx(6 hunks)apps/webapp/app/components/primitives/TextLink.tsx(2 hunks)apps/webapp/tailwind.config.js(1 hunks)
🔇 Additional comments (14)
apps/webapp/app/components/primitives/Table.tsx (7)
2-2: Nice addition for navigation!
Importing useNavigate from Remix allows dynamic navigation within the table row interactions. This is consistent with the new pattern of passing a “to” prop to navigate.
47-47: Clarify why the table header uses tabIndex={-1}.
This makes the header non-tabbable, which could be intentional for accessibility. However, confirm that this is the desired behavior since it prevents keyboard users from focusing on the header.
75-76: Good approach for adding navigation and click handlers.
Adding “to” and “onClick” props expands the row’s interactivity. Just be sure downstream usage enforces consistent typing for event handling across row-based navigations.
102-108: Keyboard accessibility looks great.
Handling the “Enter” key to trigger the same logic as onClick is a clean approach and ensures compliance with accessible interactions. Great job here.
236-237: Conditional cursor-pointer is a good pattern.
Assigning “cursor-pointer” only when hasAction is true prevents confusion for non-clickable cells. Good approach for usability.
245-245: Directly rendering children is simpler.
By eliminating conditional logic wrapping Link or button, you now rely on the row for navigation or the consumer to handle wrapping. This is cleaner, but confirm that all needed ARIA attributes are present if these children represent interactive elements.
80-101: Potential child type checking caveat.
You’re using child.type === TableCell to detect cells with a “to” prop. This could fail if TableCell is wrapped by HOCs or if multiple React instances are in play.
If you want to confirm that no HOC or different React instance is wrapping TableCell, run the following:
apps/webapp/app/components/primitives/TextLink.tsx (2)
7-7: LGTM: Focus-visible styles added consistently
The addition of focus-visible:focus-custom to both primary and secondary variants aligns with the PR objectives for more refined focus states.
Also applies to: 9-9
37-37: LGTM: Icon sizing updated to modern notation
The change from h-4 w-4 to size-4 follows modern Tailwind practices for consistent sizing across components.
Also applies to: 44-44
apps/webapp/app/components/primitives/Input.tsx (2)
7-7: LGTM: Focus ring refinements align with objectives
The changes to focus ring styles:
- Reduced thickness from ring-2 to ring-1
- Updated color to ring-charcoal-650 for subtler focus states
These changes align perfectly with the PR objectives for more refined focus states.
20-20: LGTM: Consistent icon sizing across variants
Updated all icon size classes to use the modern size-X notation, maintaining consistency across all variants and aligning with the changes in TextLink component.
Also applies to: 27-27, 34-34, 40-40
apps/webapp/app/components/primitives/SimpleSelect.tsx (1)
32-32: LGTM: Focus-visible state added as per objectives
Replaced explicit ring styles with focus-visible:focus-custom, which:
- Adds the missing focus-visible state as specified in PR objectives
- Maintains consistency with other components using the same focus style
apps/webapp/tailwind.config.js (2)
215-218: LGTM: Focus style changes align with PR objectives
The changes successfully implement more subtle focus styles by:
- Reducing outline thickness from 2px to 1px
- Using the textLink color for consistent branding
- Adjusting border radius for better visual harmony
Let's verify the impact on components mentioned in the PR:
✅ Verification successful
Focus style changes successfully implemented across components
The changes to focus styles in tailwind.config.js have been properly implemented and are being used consistently:
- SimpleSelect and TextLink components now use
focus-visible:focus-customas required - Input component has updated focus styles with 1px ring thickness
- The changes are applied across multiple primitive components ensuring consistent focus states
- The textLink color is properly used through the focus-custom utility class
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for components using focus-custom styles
rg -l "focus-custom" --type tsx
Length of output: 59
Script:
#!/bin/bash
# Search for components using focus-custom styles with broader file types
rg -l "focus-custom" --type ts --type jsx --type js --type tsx
# Also search for focus-visible related styles as mentioned in PR objectives
rg -l "focus-visible" --type ts --type jsx --type js --type tsx
# Search in CSS/SCSS files as well
rg -l "focus-custom|focus-visible" --type css --type scss
Length of output: 271
Script:
#!/bin/bash
# Search for components using focus-custom styles without specifying file types
rg -l "focus-custom"
# Search for focus-visible related styles
rg -l "focus-visible"
# Use fd to find relevant files first
fd -e tsx -e jsx -e ts -e js -e css -e scss
Length of output: 67219
Script:
#!/bin/bash
# Search for components using focus-custom and focus-visible styles in the codebase
rg -l "focus-custom|focus-visible" apps/webapp/app/components/
# Check the content of SimpleSelect and TextLink components mentioned in PR objectives
rg -A 5 "focus-custom|focus-visible" apps/webapp/app/components/primitives/SimpleSelect.tsx apps/webapp/app/components/primitives/TextLink.tsx
# Check the Task input component for focus state changes
rg -A 5 "focus-custom|focus-visible" apps/webapp/app/components/primitives/Input.tsx
Length of output: 3871
Line range hint 8-31: Verify contrast ratios for focus indicators
The new lavender[400] color (#826dff) is used for focus indicators. While the color choice looks good, we should verify it meets WCAG accessibility guidelines for focus indicators.
Let's check the contrast ratios:
Also applies to: 89-108
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
apps/webapp/app/components/primitives/Table.tsx (4)
83-101: Refactor row-click logic for clarity.The handleInteraction function is robust, but it nests multiple conditions. Consider extracting the button/link detection and path resolution logic into smaller helper functions to clarify the flow and reduce complexity.
102-109: Consider supporting Space key activation.Only triggering on Enter is standard, but some users also expect Space to activate the focused row. Consider extending the handleKeyDown to detect Space for improved accessibility.
117-117: Verify focus-visible style with custom backgrounds.The focus-visible:bg-background-bright class is effective for emphasizing the row. Just confirm the color doesn't clash with selected or hover states.
234-236: Cursor pointer usage.Conditionally applying cursor-pointer only when hasAction is true preserves the expected visual clue. Ensure you also handle pointer events if the row is disabled or if no actual navigation occurs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/components/primitives/Table.tsx(6 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx(1 hunks)
🔇 Additional comments (9)
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx (1)
463-463: Check for consistency in element height.
Setting a fixed height of h-12 might misalign with adjacent elements or text content. Confirm that this is consistent with the application's design guidelines and that it properly accommodates any dynamic content or text overflow.
apps/webapp/app/components/primitives/Table.tsx (8)
2-3: Imports reorganized for new navigation approach.
The introduction of useNavigate and updated React imports is good. Ensure that all references to Link or anchor tags are replaced or removed if they're no longer in use.
47-47: Confirm necessity of tabIndex on thead.
Assigning tabIndex={-1} to a header may be unusual unless there's a specific accessibility or keyboard navigation reason. Verify this doesn’t create unintended behavior for screen readers or tab flow.
75-76: Flexible row-level navigation props.
Allowing to? and onClick? in TableRowProps makes the component more versatile by enabling multiple interaction patterns. This looks good.
80-82: Use of useNavigate for programmatic navigation.
This is a neat approach for row-based navigation. Keep in mind server-side rendering constraints if used in non-browser contexts.
113-114: Dynamic tabIndex for disabled rows.
Using tabIndex={disabled ? -1 : 0} is a great practice, ensuring interactive rows are focusable while disabled ones are skipped. This is good for accessibility.
163-163: tabIndex={-1} for header cells.
This prevents accidentally tabbing into a header cell. It’s typically fine, but confirm that no interactive elements (e.g., tooltips) rely on keyboard focus.
192-192: Hover and focus styling logic.
The rowHoverStyles merges well with group-hover interactions. Verify that grouping doesn’t override custom focus states if you plan to highlight a selected row or handle user interactions.
244-244: Solid approach for rendering children directly.
Rendering children without wrapping them in an additional interactive element clarifies the cell’s content model and focuses interactivity at the row level.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/webapp/app/components/runs/v3/RunTag.tsx (1)
Line range hint
6-30: Consider enhancing keyboard accessibilityGiven this PR's focus on accessibility improvements, consider making the tag component keyboard interactive if it serves as a clickable/interactive element in any context. This would involve:
- Adding appropriate role attribute
- Making it focusable if interactive
- Adding keyboard event handlers
Example enhancement:
export function RunTag({ tag }: { tag: string }) { const tagResult = useMemo(() => splitTag(tag), [tag]); + const isInteractive = false; // determine based on usage context + + const commonProps = isInteractive ? { + role: "button", + tabIndex: 0, + onKeyDown: (e: React.KeyboardEvent) => { + if (e.key === "Enter" || e.key === " ") { + // Handle interaction + } + } + } : {}; if (typeof tagResult === "string") { return ( - <span className="flex h-6 items-stretch"> + <span className="flex h-6 items-stretch" {...commonProps}>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/webapp/app/components/runs/v3/RunTag.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx(1 hunks)
🔇 Additional comments (3)
apps/webapp/app/components/runs/v3/RunTag.tsx (1)
25-27: LGTM! Consider verifying overflow behavior
The addition of whitespace-nowrap is a good improvement to prevent awkward text wrapping in tag values. However, ensure that the parent container handles overflow appropriately when tag values are long.
✅ Verification successful
Based on the search results, I can see that the parent components of RunTag have proper overflow handling:
-
The RunTag component is used within containers that have overflow handling classes:
overflow-x-autofor horizontal scrollingoverflow-y-autofor vertical scrollingoverflow-hiddenfor containing the layout
-
The parent containers also use proper scrollbar styling and grid layouts to manage space effectively.
LGTM! The whitespace-nowrap addition is safe
The addition of whitespace-nowrap is well-supported by parent containers that properly handle overflow through scrolling mechanisms. The tag values will remain intact without causing layout issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Look for potential overflow handling in parent components
# Search for usage of RunTag to verify parent container styles
rg -l "RunTag" | xargs rg -A 5 -B 5 "RunTag"
# Search for potential overflow handling classes near RunTag usage
rg -l "RunTag" | xargs rg "overflow-\w+|truncate"
Length of output: 38513
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx (2)
227-227: LGTM! Improved spacing between search input and button.
The addition of gap-1 to the flex container provides consistent and appropriate spacing between the search input and the toggle button, improving the visual hierarchy.
Line range hint 227-234: Consider keyboard navigation implications of autoFocus.
While the autoFocus on the search input aligns with the PR's focus state refinements, ensure this doesn't interfere with users who rely on keyboard navigation to reach other important elements first.
Consider adding a skip link if there are important navigation elements that should be accessible before the search input.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/components/runs/v3/ScheduleFilters.tsx (1)
106-106: Ensure consistent spacing between filters.Adding "ml-2" provides extra left margin for the first filter. Check if you should align margins for other filters to maintain a uniform look.
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (1)
195-199: Remove the unnecessary Fragment.
Wrapping a single child inside a Fragment is unnecessary. You can safely remove it to address the linter warning and simplify the code.Apply this diff to remove the extra Fragment:
- <> <TableHeaderCell>Compute</TableHeaderCell> - </> + <TableHeaderCell>Compute</TableHeaderCell>🧰 Tools
🪛 Biome (1.9.4)
[error] 195-197: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
apps/webapp/app/components/primitives/Table.tsx (1)
113-117: Conditional focus for disabled rows.
Using “tabIndex={disabled ? -1 : 0}” properly excludes disabled rows from the tab sequence, improving UX for assistive technologies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
apps/webapp/app/components/primitives/Table.tsx(7 hunks)apps/webapp/app/components/runs/v3/ScheduleFilters.tsx(2 hunks)apps/webapp/app/components/runs/v3/TaskRunsTable.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx(6 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.apikeys/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.concurrency/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.environment-variables/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules/route.tsx(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.v3.usage/route.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.environment-variables/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.apikeys/route.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx
[error] 195-197: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (17)
apps/webapp/app/components/runs/v3/ScheduleFilters.tsx (1)
96-96: Consider verifying if removing "flex-row" is intentional.
By default, the "flex" class in Tailwind is laid out as a row. Removing "flex-row" may be a no-op. Validate if you intended to maintain row-based layout or switch to a different direction.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.concurrency/route.tsx (1)
83-86: Streamlined table header structure.
Directly placing TableHeaderCell elements under TableHeader (without TableRow) is consistent with similar refactors in the codebase, reduces nesting, and improves clarity.
apps/webapp/app/routes/_app.orgs.$organizationSlug.v3.usage/route.tsx (1)
210-215: Consistent header cells for numeric data.
Aligning the numeric data columns (Runs, cost, duration) to the right is a best practice, improving readability and consistency. No issues found.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments/route.tsx (1)
123-153: Clearer header structure & helpful Status tooltip.
This flattened TableHeader structure, with a descriptive tooltip for deployment statuses, enhances both readability and usability. Ensure that tooltip content remains keyboard-accessible for screen readers.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules/route.tsx (4)
68-68: New cn utility import.
Importing the cn() utility for conditional classNames is a clean approach to dynamic styling.
214-219: Dynamic grid layout for schedule table container.
Good use of cn() to conditionally adjust the grid rows based on totalPages, improving layout responsiveness.
221-226: Conditional pagination styling.
Applying conditional styles for pagination controls only when multiple pages exist is a neat solution to avoid unnecessary space or borders.
388-431: Informative tooltip explaining schedule types.
The tooltip that distinguishes between DECLARATIVE and IMPERATIVE schedules is well structured, giving users a clear overview. Consider verifying its keyboard accessibility to ensure an inclusive experience.
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (2)
109-130: Selection column logic looks good.
The checkbox handling for bulk selection is straightforward and appears to integrate well with the existing selection provider. No issues spotted.
250-270: Tooltip and tagging implementation are well-structured.
The inline documentation and reference links for using tags ensure excellent discoverability.
apps/webapp/app/components/primitives/Table.tsx (7)
2-3: Good switch to programmatic navigation.
Importing and using useNavigate fosters flexibility if you need to dynamically control routing, rather than relying solely on static links.
75-76: Enhanced interactivity with “to” and “onClick” props.
The new props and the handling logic allow table rows to be navigable via Enter key and clickable if they have a cell with a “to” prop. This is a solid approach for making rows function like links but still remain accessible.
Also applies to: 80-101, 102-108
192-192: Hover styles are well-organized.
Utilizing these keyed style variants fosters consistency. No issues here.
234-236: Optional cursor pointer for actionable cells.
Conditionally applying “cursor-pointer” when “hasAction” is true clarifies user interactivity. This is a clean approach.
316-316: Absolute positioning for the popover container.
Positioning the popover interaction elements with “absolute” ensures they stay properly aligned. Looks good.
47-47: Tab index for the header.
Be sure that the negative tab index is intentional. It is useful for programmatic focus, but it will not be keyboard-focusable in normal tab order.
✅ Verification successful
Let me gather more context about the tabIndex usage in the Table component and related components.
Let me try again with corrected shell scripts to gather the necessary context.
Negative tabIndex is intentionally used for proper keyboard navigation
The negative tabIndex in the TableHeader component is part of a well-structured keyboard navigation pattern where:
- The table header (thead) has
tabIndex={-1}to prevent it from being tab-focusable - Individual table rows have
tabIndex={disabled ? -1 : 0}to make them focusable when not disabled - Table header cells also have
tabIndex={-1}to ensure focus remains on interactive elements
This implementation follows accessibility best practices by maintaining a logical tab order through the table's interactive elements while preventing unnecessary focus stops on structural elements.
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Check the Table component implementation and its context
rg -A 5 -B 5 "tabIndex={-1}" apps/webapp/app/components/primitives/Table.tsx
# Look for other instances of tabIndex usage in the codebase
rg "tabIndex" --type tsx --type jsx
# Check for any accessibility-related comments or documentation
rg -i "accessibility|a11y|keyboard|focus" apps/webapp/app/components/primitives/Table.tsx
Length of output: 1289
Script:
#!/bin/bash
# Check the Table component implementation and its context
rg -A 5 -B 5 'tabIndex="-1"' apps/webapp/app/components/primitives/Table.tsx
# Look for the Table component's header implementation
cat apps/webapp/app/components/primitives/Table.tsx
# Look for other instances of tabIndex usage in the codebase
rg 'tabIndex' .
Length of output: 12713
163-163: Header cell focus approach.
Adding “tabIndex={-1}” can help with advanced keyboard or screen reader setups, but confirm whether focusing table headers is necessary.
✅ Verification successful
Let me gather more information about the table component and its accessibility implementation to better understand the context of the tabIndex usage.
This will help us understand:
- The context around where tabIndex={-1} is being used
- The accessibility implementation details
- Any keyboard interaction patterns
Let me try another search with fixed regex to better understand the table header implementation:
Based on the search results, I can now provide a final response as we have enough context about the table header implementation:
Table header's tabIndex={-1} is correctly implemented for accessibility
The tabIndex={-1} on the thead element is appropriate as it's part of a well-structured table where:
- The table uses proper semantic elements (thead, th with scope="col")
- Header cells are properly marked with
thelements - The component includes keyboard navigation support (onKeyDown handlers)
- The implementation follows accessibility patterns with proper ARIA attributes
🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Let's examine the Table component implementation and its accessibility patterns
rg -A 5 -B 5 "tabIndex={-1}" apps/webapp/app/components/primitives/Table.tsx
Length of output: 184
Script:
#!/bin/bash
# Look for any ARIA attributes or accessibility-related patterns in the Table component
rg -A 3 -B 3 "aria-|role=" apps/webapp/app/components/primitives/Table.tsx
Length of output: 302
Script:
#!/bin/bash
# Check if there are any keyboard navigation handlers
rg -A 3 -B 3 "onKeyDown|onKeyPress|onKeyUp" apps/webapp/app/components/primitives/Table.tsx
Length of output: 474
Script:
#!/bin/bash
# Search for table header implementation with fixed regex
rg -A 5 -B 5 'tabIndex="-1"' apps/webapp/app/components/primitives/Table.tsx
Length of output: 78
Script:
#!/bin/bash
# Look for TableHeader or Th component implementation
ast-grep --pattern 'const Th = $$$'
Length of output: 3735
Script:
#!/bin/bash
# Check for any header-related accessibility implementations
rg -A 3 -B 3 "th|thead|TableHead" apps/webapp/app/components/primitives/Table.tsx
Length of output: 4078
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx
Outdated
Show resolved
Hide resolved
apps/webapp/app/routes/_app.orgs.$organizationSlug.v3.usage/route.tsx
Outdated
Show resolved
Hide resolved
.../webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules/route.tsx
Outdated
Show resolved
Hide resolved
...ebapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.concurrency/route.tsx
Outdated
Show resolved
Hide resolved
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.apikeys/route.tsx
Outdated
Show resolved
Hide resolved
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx
Outdated
Show resolved
Hide resolved
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (1)
Line range hint
286-301: Enhance keyboard navigation for table rows.According to the PR objectives, users should be able to select rows using the Enter key. Consider adding Enter key handling to improve accessibility.
<TableRow to={path} key={run.id}> {allowSelection && ( <TableCell className="pl-3 pr-0"> <Checkbox checked={has(run.id)} onChange={(element) => { toggle(run.id); }} ref={(r) => { checkboxes.current[index + 1] = r; }} - onKeyDown={(event) => navigateCheckboxes(event, index + 1)} + onKeyDown={(event) => { + if (event.key === "Enter") { + toggle(run.id); + } + navigateCheckboxes(event, index + 1); + }} /> </TableCell> )}
🧹 Nitpick comments (4)
apps/webapp/app/components/primitives/Table.tsx (3)
137-138: Consider dynamic ARIA role based on navigation typeThe
role="link"might be misleading when the row only has anonClickhandler without atoprop. Consider using a dynamic role:- role="link" + role={to ? "link" : "button"}
126-132: Enhance keyboard interaction with Space key supportConsider adding Space key support for activation, which is a common accessibility pattern for interactive elements:
const handleKeyDown = (event: React.KeyboardEvent) => { - if (event.key === "Enter") { + if (event.key === "Enter" || event.key === " ") { event.preventDefault(); event.stopPropagation(); handleNavigation(event); } };
266-266: Improve maintainability of pointer-events selectorsConsider extracting the pointer-events selectors into a CSS custom property for better maintainability:
- "child:pointer-events-none [&>[role=button]]:pointer-events-auto [&>[role=menu]]:pointer-events-auto [&>a]:pointer-events-auto [&>button]:pointer-events-auto", + "[--interactive-selectors:role=button,role=menu,a,button] child:pointer-events-none [&>:is([var(--interactive-selectors)])]:pointer-events-auto",This approach:
- Reduces repetition
- Makes it easier to maintain the list of interactive elements
- Improves readability
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (1)
195-198: Remove unnecessary Fragment.The Fragment wrapper around the
TableHeaderCellis redundant as it contains only one child.- <> - <TableHeaderCell>Compute</TableHeaderCell> - </> + <TableHeaderCell>Compute</TableHeaderCell>🧰 Tools
🪛 Biome (1.9.4)
[error] 195-197: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
apps/webapp/app/components/primitives/Table.tsx(7 hunks)apps/webapp/app/components/runs/v3/TaskRunsTable.tsx(6 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx(8 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.batches/route.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.batches/route.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx
[error] 195-197: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
🔇 Additional comments (5)
apps/webapp/app/components/primitives/Table.tsx (2)
2-2: LGTM! Navigation and accessibility improvementsThe switch from Link to useNavigate and making the header row non-focusable are good improvements for keyboard navigation and accessibility.
Also applies to: 48-48
142-142: LGTM! Well-implemented focus-visible stylesThe focus and hover styles are well-implemented with proper CSS nesting and maintain a good balance between visibility and subtlety, aligning perfectly with the PR objectives.
Also applies to: 218-218
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (3)
109-273: Ensure proper table structure withtheadelement.The table header cells should be wrapped in a
theadelement to maintain proper HTML table structure.🧰 Tools
🪛 Biome (1.9.4)
[error] 195-197: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
Line range hint
386-590: Well-implemented action cells and helper components!The implementation shows good attention to accessibility with proper tooltips, clear action labels, and appropriate icon usage.
🧰 Tools
🪛 Biome (1.9.4)
[error] 195-197: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
Line range hint
1-590: Verify implementation of all PR objectives.While this file implements the table interaction improvements, please ensure that the following PR objectives are addressed in other files:
- Subtle focus styles (1px border thickness)
- Task input field focus state
- SimpleSelect and TextLink focus-visible states
🧰 Tools
🪛 Biome (1.9.4)
[error] 195-197: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
apps/webapp/app/components/events/EventsTable.tsx (1)
63-65: Consider enhancing the screen reader experience.While the "Go to page" screen reader text is good, consider adding more context about what the column represents and how to interact with it.
- <span className="sr-only">Go to page</span> + <span className="sr-only">Navigate to event details</span>apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules.$scheduleParam/route.tsx (1)
329-329: Consider rewording the placeholder text “You found a bug.”
This user-facing message might cause confusion or alarm. A more descriptive yet reassuring text could enhance the user experience.- <PlaceholderText title="You found a bug" /> + <PlaceholderText title="No upcoming runs found" />apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (1)
198-200: Remove redundant Fragment to reduce complexity.
Here’s a suggested fix for the static analysis warning about unnecessary fragments:{showCompute && ( - <> - <TableHeaderCell>Compute</TableHeaderCell> - </> + <TableHeaderCell>Compute</TableHeaderCell> )}🧰 Tools
🪛 Biome (1.9.4)
[error] 198-200: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment(lint/complexity/noUselessFragments)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
apps/webapp/app/components/events/EventsTable.tsx(1 hunks)apps/webapp/app/components/jobs/JobsTable.tsx(1 hunks)apps/webapp/app/components/primitives/Table.tsx(11 hunks)apps/webapp/app/components/runs/RunsTable.tsx(1 hunks)apps/webapp/app/components/runs/v3/TaskRunsTable.tsx(5 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.http-endpoints.$httpEndpointParam/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.http-endpoints._index/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.triggers._index/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments.$deploymentParam/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules.$scheduleParam/route.tsx(3 hunks)
✅ Files skipped from review due to trivial changes (5)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.triggers._index/route.tsx
- apps/webapp/app/components/runs/RunsTable.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.http-endpoints._index/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.$projectParam.http-endpoints.$httpEndpointParam/route.tsx
- apps/webapp/app/components/jobs/JobsTable.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments.$deploymentParam/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx
🧰 Additional context used
🪛 Biome (1.9.4)
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx
[error] 198-200: Avoid using unnecessary Fragment.
A fragment is redundant if it contains only one child, or if it is the child of a html element, and is not a keyed fragment.
Unsafe fix: Remove the Fragment
(lint/complexity/noUselessFragments)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: typecheck / typecheck
- GitHub Check: units / 🧪 Unit Tests
🔇 Additional comments (7)
apps/webapp/app/components/events/EventsTable.tsx (1)
55-65: Verify keyboard navigation behavior with the new table structure.The removal of the
TableRowwrapper in the header is a structural improvement. However, given the PR's focus on keyboard navigation, please ensure:
- Tab navigation works correctly through the table headers
- Enter key functions properly for row selection
- Focus states are visible and consistent with the new 1px border style mentioned in the PR objectives
Let's verify the table's structure and accessibility:
✅ Verification successful
Table structure and keyboard navigation are properly implemented
The Table primitive component implements robust keyboard navigation and focus management:
- Header structure maintains semantic HTML with proper tabindex
- Interactive elements have visible focus states
- Screen reader support is preserved
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if Table components consistently implement keyboard navigation ast-grep --pattern 'const Table = $_({$$$}) => { $$$ onKeyDown={$_} $$$ }' # Check for focus-visible styles in the related CSS/Tailwind files rg -l "focus-visible" --type css rg -l "focus-visible" --type tsLength of output: 893
Script:
#!/bin/bash # Check Table primitive implementation cat apps/webapp/app/components/primitives/Table.tsx # Check how EventsTable uses Table primitive cat apps/webapp/app/components/events/EventsTable.tsxLength of output: 17465
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules.$scheduleParam/route.tsx (2)
223-223: The subtle padding addition is consistent with the refined focus visuals.
This one-line style tweak aligns well with the PR’s objective to refine the UI’s subtle focus changes.
289-303: Use of “bright” table variant complements the new UI design.
Applyingvariant="bright"inTaskRunsTablehelps achieve a lighter visual style for focus states.apps/webapp/app/components/primitives/Table.tsx (2)
39-56: Context API usage for table variants is well-implemented.
IntroducingTableContextand wrapping the<table>element inside<TableContext.Provider>is a clean solution for applying variant-based styles.
247-251: Validate CMD+click behavior when using clickable table cells.
In earlier reviews, concern was raised about preserving CMD+click to open links in a new tab. Please confirm that your handling oftabIndexandLinkorbuttonelements doesn’t interfere with CMD+click.#!/bin/bash # Description: Grep for potential usage of table cells with clickable links and confirm they do not override default browser meta-key actions (CMD+click). rg --context 5 "onClick|to="Also applies to: 255-259
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx (2)
Line range hint
62-72: Table variant property offers consistent display options.
Allowing differentTableVariantchoices for the runs table aligns with the broader styling changes introduced in the PR.
304-304:isTabbableCellnicely enhances keyboard accessibility.
This property subtly improves navigation for keyboard users, consistent with a focus-friendly design.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
apps/webapp/app/components/primitives/Table.tsx (2)
8-27: LGTM! Consider adding JSDoc comments for the variants object.The variants implementation effectively manages focus states and provides consistent styling. The use of group-has selectors for focus states is a modern approach.
Consider adding JSDoc comments to document the purpose and usage of each variant:
/** * Defines visual variants for the table components. * - bright: Used for high contrast scenarios with prominent focus states * - dimmed: Used for regular table displays with subtle focus states */ const variants = { // ... rest of the code
Line range hint
191-265: Consider making tabIndex logic more explicit.The implementation successfully adds keyboard navigation support, but the tabIndex logic could be more explicit.
Consider extracting the tabIndex logic to a constant for better readability:
export const TableCell = forwardRef<HTMLTableCellElement, Omit<TableCellProps, "variant">>( ({ // ... other props isTabbableCell = false, }, ref) => { + const cellTabIndex = isTabbableCell ? 0 : -1; // ... other code return ( <td> {to ? ( <Link to={to} className={cn("cursor-pointer focus:outline-none", flexClasses, actionClassName)} - tabIndex={isTabbableCell ? 0 : -1} + tabIndex={cellTabIndex} > {children} </Link> ) : onClick ? ( <button onClick={onClick} className={cn("cursor-pointer focus:outline-none", flexClasses, actionClassName)} - tabIndex={isTabbableCell ? 0 : -1} + tabIndex={cellTabIndex} > {children} </button> ) : ( <>{children}</> )} </td> ); } );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
apps/webapp/app/components/primitives/Table.tsx(11 hunks)apps/webapp/app/components/runs/v3/TaskRunsTable.tsx(5 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx(6 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.batches/route.tsx(2 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments.$deploymentParam/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments/route.tsx(1 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules.$scheduleParam/route.tsx(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules/route.tsx(3 hunks)apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.batches/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments.$deploymentParam/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments/route.tsx
- apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules.$scheduleParam/route.tsx
- apps/webapp/app/components/runs/v3/TaskRunsTable.tsx
🔇 Additional comments (5)
apps/webapp/app/components/primitives/Table.tsx (5)
38-56: LGTM! Clean context implementation.The TableContext implementation effectively manages the variant state across the table components, following React context best practices.
Line range hint
67-82: LGTM! Good use of context for variant management.The TableHeader implementation correctly uses context for variant management while maintaining proper accessibility structure.
Line range hint
108-123: LGTM! Accessible focus implementation.The focus styles implementation maintains accessibility while achieving the PR objective of making focus states more subtle.
Line range hint
296-389: LGTM! Consistent styling with other components.The TableCellMenu implementation maintains consistency with other components while properly handling menu button states and variants.
Line range hint
191-265: Verify Enter key handling implementation.While the code implements tabIndex management for keyboard navigation, the handling of Enter key events for row selection isn't explicitly visible in this file.
| ); | ||
| } | ||
| ); | ||
|
|
||
| type TableHeaderProps = { | ||
| className?: string; | ||
| children: ReactNode; | ||
| variant?: TableVariant; |
There was a problem hiding this comment.
Does this still need to be here?
There was a problem hiding this comment.
This variant means you can pass "bright" or "dimmed" to the table to have it display nicely on bright backgrounds (like in the side inspector panels).
| @@ -71,17 +102,19 @@ type TableRowProps = { | |||
| children: ReactNode; | |||
| disabled?: boolean; | |||
| isSelected?: boolean; | |||
| variant?: TableVariant; | |||
There was a problem hiding this comment.
Does this still need to be here?
| isSelected?: boolean; | ||
| isTabbableCell?: boolean; | ||
| variant?: TableVariant; |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
apps/webapp/app/components/primitives/Table.tsx (2)
8-29: LGTM! Consider adding JSDoc comments.The variants system is well-structured and provides consistent styling for both bright and dimmed themes. The implementation aligns well with the PR's focus on refining focus states.
Consider adding JSDoc comments to document the purpose and usage of each variant:
/** * Defines the visual variants for table components. * - bright: Used for tables on bright backgrounds (e.g., side inspector panels) * - dimmed: Default variant for standard table displays */ const variants = { // ... rest of the code } as const;
244-256: Consider adding aria-current for selected items.For better screen reader support, consider adding
aria-current="true"when the cell is selected.<Link to={to} className={cn("cursor-pointer focus:outline-none", flexClasses, actionClassName)} tabIndex={isTabbableCell ? 0 : -1} + aria-current={isSelected ? "true" : undefined} > {children} </Link>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/webapp/app/components/primitives/Table.tsx(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - pnpm)
- GitHub Check: e2e / 🧪 CLI v3 tests (windows-latest - npm)
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - pnpm)
- GitHub Check: typecheck / typecheck
- GitHub Check: e2e / 🧪 CLI v3 tests (buildjet-8vcpu-ubuntu-2204 - npm)
- GitHub Check: units / 🧪 Unit Tests
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
apps/webapp/app/components/primitives/Table.tsx (5)
38-56: LGTM! Clean context implementation.The TableContext implementation follows React best practices, providing a clean way to manage variants across the table components.
68-74: LGTM! Robust header implementation.The TableHeader component correctly implements variant styles while maintaining proper sticky positioning.
184-185: LGTM! Good accessibility improvement.The addition of
isTabbableCellprop improves keyboard navigation by allowing fine-grained control over focusable elements.Also applies to: 201-201
334-344: LGTM! Clean menu styling implementation.The menu button styling is well-integrated with the variant system and provides consistent hover states.
108-117: Verify keyboard navigation with outline-none.While the implementation looks good, the
outline-noneclass might affect keyboard navigation. Please verify that the custom focus indicators provide sufficient visibility for keyboard users.✅ Verification successful
Keyboard navigation implementation is robust and accessible ✅
The
outline-noneis properly compensated with custom focus indicators including:
- Background color changes via
group-has-[[tabindex='0']:focus]- Visual borders using pseudo-elements
- Proper tabIndex management for keyboard navigation
- Focus-visible states for keyboard users
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing accessibility tests or documentation rg -l "accessibility|a11y|keyboard|focus" --type mdLength of output: 53
Script:
#!/bin/bash # Search for focus-related styles and keyboard handlers in Table components ast-grep --pattern 'className={$_focus$_}' rg "onKey|focus|tabIndex" apps/webapp/app/components/primitives/Table.tsx -A 2 rg "aria-" apps/webapp/app/components/primitives/Table.tsx -A 2Length of output: 2849
| "text-xs text-charcoal-400 has-[[tabindex='0']:focus]:before:absolute has-[[tabindex='0']:focus]:before:-top-px has-[[tabindex='0']:focus]:before:left-0 has-[[tabindex='0']:focus]:before:h-px has-[[tabindex='0']:focus]:before:w-3 has-[[tabindex='0']:focus]:before:bg-grid-dimmed has-[[tabindex='0']:focus]:after:absolute has-[[tabindex='0']:focus]:after:bottom-0 has-[[tabindex='0']:focus]:after:left-0 has-[[tabindex='0']:focus]:after:right-0 has-[[tabindex='0']:focus]:after:h-px has-[[tabindex='0']:focus]:after:bg-grid-dimmed", | ||
| variants[variant].cell, | ||
| to || onClick || hasAction ? "cursor-pointer" : "cursor-default px-3 py-3 align-middle", | ||
| !to && !onClick && alignmentClassName, | ||
| isSticky && stickyStyles, | ||
| isSelected && isSelectedStyle, | ||
| !isSelected && rowHoverStyles[rowHoverStyle], | ||
| isSticky && | ||
| "[&:has(.group-hover/table-row:block)]:w-auto sticky right-0 bg-background-dimmed", | ||
| isSticky && variants[variant].stickyCell, | ||
| isSelected && variants[variant].rowSelected, | ||
| !isSelected && | ||
| "group-hover/table-row:before:absolute group-hover/table-row:before:left-0 group-hover/table-row:before:top-[-1px] group-hover/table-row:before:h-px group-hover/table-row:before:w-3 group-hover/table-row:before:bg-charcoal-750 group-hover/table-row:after:absolute group-hover/table-row:after:bottom-0 group-hover/table-row:after:left-0 group-hover/table-row:after:h-px group-hover/table-row:after:w-3 group-hover/table-row:after:bg-charcoal-750 group-focus-visible/table-row:bg-background-bright", |
There was a problem hiding this comment.
💡 Codebase verification
Focus indicators need higher contrast for accessibility.
The current focus indicator using bg-grid-dimmed (#212327) against the background has insufficient contrast. To meet WCAG 2.1 requirements, focus indicators must have at least a 3:1 contrast ratio. Consider using a higher contrast color like charcoal-400 (#878C99) or charcoal-300 (#B5B8C0) for better visibility.
🔗 Analysis chain
Verify focus indicator contrast.
The custom focus indicators use subtle colors. Please ensure they meet WCAG contrast requirements for accessibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any color contrast testing setup or documentation
rg -l "contrast|wcag|a11y" --type css --type md
Length of output: 49
Script:
#!/bin/bash
# Search for Tailwind config files and theme definitions
fd -e js -e ts -e json tailwind.config
fd -e js -e ts -e json theme.
Length of output: 200
Script:
#!/bin/bash
# Search for color definitions in the codebase
rg -g '!*.test.*' -g '!*.spec.*' "grid-dimmed|charcoal-750" --type ts --type js --type json
Length of output: 22394
Script:
#!/bin/bash
# Check Tailwind config for color definitions
cat apps/webapp/tailwind.config.js
Length of output: 7016
Focus styles are slightly more subtle
1px border instead of 2px
More subtle focus state for the Task input as it's auto-focused on page load
Tabbing down the table now selects the rows. Enter key to select
Added missing focus-visible states for
Summary by CodeRabbit
Release Notes
Style Updates
Accessibility Improvements
isTabbableCellproperties.Design System
Minor Enhancements