Skip to content
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

Focus visible style refinements #1577

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

samejr
Copy link
Member

@samejr samejr commented Dec 18, 2024

Focus styles are slightly more subtle

1px border instead of 2px
CleanShot 2024-12-18 at 13 40 50@2x

More subtle focus state for the Task input as it's auto-focused on page load

CleanShot 2024-12-18 at 13 41 53@2x

Tabbing down the table now selects the rows. Enter key to select

CleanShot 2024-12-18 at 13 44 16@2x

Added missing focus-visible states for

  • SimpleSelect
  • TextLink

Summary by CodeRabbit

  • New Features

    • Enhanced focus styles for accessibility in input and link components.
    • Improved interactivity and accessibility in table components with new navigation props.
    • New color palettes and responsive design options added to the Tailwind CSS configuration.
    • Enhanced functionality for managing project alerts with improved UI structure and feedback mechanisms.
    • Adjusted layout and spacing in the task management interface for better visual presentation.
  • Bug Fixes

    • Adjusted focus styling for select trigger components to improve user experience.
  • Style

    • Updated CSS class names and styles for various components to enhance visual presentation and feedback.
    • Added whitespace management for tag display in the RunTag component.
    • Simplified table header structures across multiple components for clarity and consistency.

Copy link

changeset-bot bot commented Dec 18, 2024

⚠️ No Changeset found

Latest commit: 4ab3f3c

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

Copy link
Contributor

coderabbitai bot commented Dec 18, 2024

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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

apps/webapp/app/components/primitives/Table.tsx

Oops! 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.

Walkthrough

This pull request introduces a series of updates across multiple UI components in the web application, focusing on CSS class name modifications, enhanced focus styles, and improved interactivity. Key components affected include Input, SimpleSelect, Table, and TextLink, with corresponding adjustments made to the Tailwind configuration to support these changes. The updates aim to enhance accessibility and create a more consistent user interface without altering the core functionality of the components.

Changes

File Change Summary
apps/webapp/app/components/primitives/Input.tsx Updated CSS class names for icon sizes and focus ring styles.
apps/webapp/app/components/primitives/SimpleSelect.tsx Modified focus styling for SelectTrigger component.
apps/webapp/app/components/primitives/Table.tsx Enhanced interactivity, added navigation and keyboard event handling, removed Link import.
apps/webapp/app/components/primitives/TextLink.tsx Added focus-visible:focus-custom class, updated icon size classes.
apps/webapp/tailwind.config.js Added new color palettes, updated theme configurations, introduced new animations and responsive design options.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.alerts/route.tsx Updated alert management functionality, added validation and error handling, adjusted rendering logic.
apps/webapp/app/components/runs/v3/RunTag.tsx Added whitespace-nowrap class to prevent text wrapping in the RunTag component.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam._index/route.tsx Adjusted layout for task search input area, added spacing class for better UI presentation.
apps/webapp/app/components/runs/v3/ScheduleFilters.tsx Modified layout and styling, added margin to SelectGroup component.
apps/webapp/app/components/runs/v3/TaskRunsTable.tsx Restructured table header for improved layout and organization.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.apikeys/route.tsx Simplified table header structure by removing unnecessary wrapper.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.concurrency/route.tsx Simplified table header structure by removing unnecessary wrapper.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.deployments/route.tsx Simplified table header structure by removing unnecessary wrapper and added tooltip for "Status".
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.environment-variables/route.tsx Simplified table header structure by removing unnecessary wrapper.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.schedules/route.tsx Added cn utility function for conditional CSS class application, modified table header structure.
apps/webapp/app/routes/_app.orgs.$organizationSlug.projects.v3.$projectParam.test/route.tsx Simplified table header structure by removing unnecessary wrapper.
apps/webapp/app/routes/_app.orgs.$organizationSlug.v3.usage/route.tsx Simplified table header structure by removing unnecessary wrapper.

Suggested Reviewers

  • matt-aitken

Poem

🐰 Hop, hop, through the code we go!
Styling tweaks make components glow
Focus rings sharp, interactions bright
Tailwind magic brings pure delight
A rabbit's touch of UI might! 🎨


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

pkg-pr-new bot commented Dec 18, 2024

@trigger.dev/build

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/build@1577

@trigger.dev/react-hooks

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/react-hooks@1577

trigger.dev

npm i https://pkg.pr.new/triggerdotdev/trigger.dev@1577

@trigger.dev/rsc

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/rsc@1577

@trigger.dev/sdk

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/sdk@1577

@trigger.dev/core

npm i https://pkg.pr.new/triggerdotdev/trigger.dev/@trigger.dev/core@1577

commit: 4ab3f3c

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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-visible

The .focus-custom utility correctly uses :focus-visible instead 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

📥 Commits

Reviewing files that changed from the base of the PR and between 21a4fab and 687ead4.

📒 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-custom as 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

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 687ead4 and 758a96d.

📒 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
apps/webapp/app/components/runs/v3/RunTag.tsx (1)

Line range hint 6-30: Consider enhancing keyboard accessibility

Given 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:

  1. Adding appropriate role attribute
  2. Making it focusable if interactive
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 758a96d and 1daafa3.

📒 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:

  1. The RunTag component is used within containers that have overflow handling classes:

    • overflow-x-auto for horizontal scrolling
    • overflow-y-auto for vertical scrolling
    • overflow-hidden for containing the layout
  2. 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

📥 Commits

Reviewing files that changed from the base of the PR and between 1daafa3 and 4ab3f3c.

📒 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:

  1. The context around where tabIndex={-1} is being used
  2. The accessibility implementation details
  3. 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 th elements
  • 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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant