-
-
Notifications
You must be signed in to change notification settings - Fork 808
Focus visible style refinements #1577
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
Changes from 41 commits
7f3994b
7020df3
dc2e063
b87bb11
5dc8d7f
a1c37d6
687ead4
149ee94
758a96d
8fd2948
1daafa3
9f577c7
6028ef3
f97c60d
3b0e6d4
4ab3f3c
f9caa9f
b239474
8a814d4
f2bdc64
47a560b
8ad7443
796e0d3
17302b8
9edd0e2
5b11f5c
2219abc
ab32e4d
2d46781
3998961
e7fd0c7
176541b
46b979e
3c0639e
2be75bf
13c9b98
7eab9a3
1243990
2087072
df3bc01
0b0c7df
cd6c416
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,47 +1,78 @@ | ||
import { ChevronRightIcon } from "@heroicons/react/24/solid"; | ||
import { Link } from "@remix-run/react"; | ||
import { ReactNode, forwardRef, useState } from "react"; | ||
import React, { ReactNode, forwardRef, useState, useContext, createContext } from "react"; | ||
import { cn } from "~/utils/cn"; | ||
import { Popover, PopoverContent, PopoverVerticalEllipseTrigger } from "./Popover"; | ||
import { InfoIconTooltip } from "./Tooltip"; | ||
|
||
const variants = { | ||
bright: { | ||
header: "bg-background-bright", | ||
cell: "group-hover/table-row:bg-charcoal-750 group-has-[[tabindex='0']:focus]/table-row:bg-charcoal-750", | ||
stickyCell: "bg-background-bright group-hover/table-row:bg-charcoal-750", | ||
menuButton: | ||
"bg-background-bright group-hover/table-row:bg-charcoal-750 group-hover/table-row:ring-charcoal-600/70 group-has-[[tabindex='0']:focus]/table-row:bg-charcoal-750", | ||
menuButtonDivider: "group-hover/table-row:border-charcoal-600/70", | ||
rowSelected: "bg-charcoal-750 group-hover/table-row:bg-charcoal-750", | ||
}, | ||
dimmed: { | ||
header: "bg-background-dimmed", | ||
cell: "group-hover/table-row:bg-charcoal-800 group-has-[[tabindex='0']:focus]/table-row:bg-background-bright", | ||
stickyCell: "group-hover/table-row:bg-charcoal-800", | ||
menuButton: | ||
"bg-background-dimmed group-hover/table-row:bg-charcoal-800 group-hover/table-row:ring-grid-bright group-has-[[tabindex='0']:focus]/table-row:bg-background-bright", | ||
menuButtonDivider: "group-hover/table-row:border-grid-dimmed", | ||
rowSelected: "bg-charcoal-750 group-hover/table-row:bg-charcoal-750", | ||
}, | ||
} as const; | ||
|
||
export type TableVariant = keyof typeof variants; | ||
|
||
type TableProps = { | ||
containerClassName?: string; | ||
className?: string; | ||
children: ReactNode; | ||
fullWidth?: boolean; | ||
}; | ||
|
||
export const Table = forwardRef<HTMLTableElement, TableProps>( | ||
({ className, containerClassName, children, fullWidth }, ref) => { | ||
// Add TableContext | ||
const TableContext = createContext<{ variant: TableVariant }>({ variant: "dimmed" }); | ||
|
||
export const Table = forwardRef<HTMLTableElement, TableProps & { variant?: TableVariant }>( | ||
({ className, containerClassName, children, fullWidth, variant = "dimmed" }, ref) => { | ||
return ( | ||
<div | ||
className={cn( | ||
"overflow-x-auto whitespace-nowrap border-t border-grid-bright scrollbar-thin scrollbar-track-transparent scrollbar-thumb-charcoal-600", | ||
containerClassName, | ||
fullWidth && "w-full" | ||
)} | ||
> | ||
<table ref={ref} className={cn("w-full", className)}> | ||
{children} | ||
</table> | ||
</div> | ||
<TableContext.Provider value={{ variant }}> | ||
<div | ||
className={cn( | ||
"overflow-x-auto whitespace-nowrap border-t scrollbar-thin scrollbar-track-transparent scrollbar-thumb-charcoal-600", | ||
containerClassName, | ||
fullWidth && "w-full" | ||
)} | ||
> | ||
<table ref={ref} className={cn("w-full", className)}> | ||
{children} | ||
</table> | ||
</div> | ||
</TableContext.Provider> | ||
); | ||
} | ||
); | ||
|
||
type TableHeaderProps = { | ||
className?: string; | ||
children: ReactNode; | ||
variant?: TableVariant; | ||
}; | ||
|
||
export const TableHeader = forwardRef<HTMLTableSectionElement, TableHeaderProps>( | ||
export const TableHeader = forwardRef<HTMLTableSectionElement, Omit<TableHeaderProps, "variant">>( | ||
({ className, children }, ref) => { | ||
const { variant } = useContext(TableContext); | ||
return ( | ||
<thead | ||
ref={ref} | ||
className={cn( | ||
"sticky top-0 z-10 bg-background-dimmed after:absolute after:bottom-0 after:left-0 after:right-0 after:h-px after:bg-grid-bright", | ||
"sticky top-0 z-10 after:absolute after:bottom-0 after:left-0 after:right-0 after:h-px after:bg-grid-bright", | ||
variants[variant].header, | ||
className | ||
)} | ||
> | ||
|
@@ -71,17 +102,19 @@ type TableRowProps = { | |
children: ReactNode; | ||
disabled?: boolean; | ||
isSelected?: boolean; | ||
variant?: TableVariant; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this still need to be here? |
||
}; | ||
|
||
export const TableRow = forwardRef<HTMLTableRowElement, TableRowProps>( | ||
export const TableRow = forwardRef<HTMLTableRowElement, Omit<TableRowProps, "variant">>( | ||
({ className, disabled, isSelected, children }, ref) => { | ||
const { variant } = useContext(TableContext); | ||
return ( | ||
<tr | ||
ref={ref} | ||
className={cn( | ||
"group/table-row relative w-full after:absolute after:bottom-0 after:left-3 after:right-0 after:h-px after:bg-grid-dimmed", | ||
"group/table-row relative w-full outline-none after:absolute after:bottom-0 after:left-3 after:right-0 after:h-px after:bg-grid-dimmed", | ||
isSelected && variants[variant].rowSelected, | ||
disabled && "opacity-50", | ||
isSelected && isSelectedStyle, | ||
className | ||
)} | ||
> | ||
|
@@ -94,7 +127,7 @@ export const TableRow = forwardRef<HTMLTableRowElement, TableRowProps>( | |
type TableCellBasicProps = { | ||
className?: string; | ||
alignment?: "left" | "center" | "right"; | ||
children: ReactNode; | ||
children?: ReactNode; | ||
colSpan?: number; | ||
}; | ||
|
||
|
@@ -125,6 +158,7 @@ export const TableHeaderCell = forwardRef<HTMLTableCellElement, TableHeaderCellP | |
className | ||
)} | ||
colSpan={colSpan} | ||
tabIndex={-1} | ||
> | ||
{hiddenLabel ? ( | ||
<span className="sr-only">{children}</span> | ||
|
@@ -147,25 +181,14 @@ type TableCellProps = TableCellBasicProps & { | |
hasAction?: boolean; | ||
isSticky?: boolean; | ||
actionClassName?: string; | ||
rowHoverStyle?: keyof typeof rowHoverStyles; | ||
rowHoverStyle?: string; | ||
isSelected?: boolean; | ||
isTabbableCell?: boolean; | ||
variant?: TableVariant; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is this needed? |
||
children?: ReactNode; | ||
}; | ||
|
||
const rowHoverStyles = { | ||
default: | ||
"group-hover/table-row:bg-charcoal-800 group-hover/table-row:before:absolute group-hover/table-row:before:bg-charcoal-750 group-hover/table-row:before:top-[-1px] group-hover/table-row:before:left-0 group-hover/table-row:before:h-px group-hover/table-row:before:w-3 group-hover/table-row:after:absolute group-hover/table-row:after:bg-charcoal-750 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", | ||
dimmed: | ||
"group-hover/table-row:bg-charcoal-850 group-hover/table-row:before:absolute group-hover/table-row:before:bg-charcoal-800 group-hover/table-row:before:top-[-1px] group-hover/table-row:before:left-0 group-hover/table-row:before:h-px group-hover/table-row:before:w-3 group-hover/table-row:after:absolute group-hover/table-row:after:bg-charcoal-800 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", | ||
bright: | ||
"group-hover/table-row:bg-charcoal-750 group-hover/table-row:before:absolute group-hover/table-row:before:bg-charcoal-700 group-hover/table-row:before:top-[-1px] group-hover/table-row:before:left-0 group-hover/table-row:before:h-px group-hover/table-row:before:w-3 group-hover/table-row:after:absolute group-hover/table-row:after:bg-charcoal-700 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", | ||
}; | ||
|
||
const stickyStyles = | ||
"sticky right-0 bg-background-dimmed group-hover/table-row:bg-charcoal-750 w-[--sticky-width] [&:has(.group-hover\\/table-row\\:block)]:w-auto"; | ||
|
||
const isSelectedStyle = "bg-charcoal-750 group-hover:bg-charcoal-750"; | ||
|
||
export const TableCell = forwardRef<HTMLTableCellElement, TableCellProps>( | ||
export const TableCell = forwardRef<HTMLTableCellElement, Omit<TableCellProps, "variant">>( | ||
( | ||
{ | ||
className, | ||
|
@@ -177,8 +200,8 @@ export const TableCell = forwardRef<HTMLTableCellElement, TableCellProps>( | |
onClick, | ||
hasAction = false, | ||
isSticky = false, | ||
rowHoverStyle = "default", | ||
isSelected, | ||
isTabbableCell = false, | ||
}, | ||
ref | ||
) => { | ||
|
@@ -193,34 +216,47 @@ export const TableCell = forwardRef<HTMLTableCellElement, TableCellProps>( | |
} | ||
|
||
const flexClasses = cn( | ||
"flex w-full whitespace-nowrap px-3 py-3 text-xs text-text-dimmed", | ||
"flex w-full whitespace-nowrap px-3 py-3 items-center text-xs text-text-dimmed", | ||
alignment === "left" | ||
? "justify-start text-left" | ||
: alignment === "center" | ||
? "justify-center text-center" | ||
: "justify-end text-right" | ||
); | ||
const { variant } = useContext(TableContext); | ||
|
||
return ( | ||
<td | ||
ref={ref} | ||
className={cn( | ||
"text-xs text-charcoal-400", | ||
to || onClick || hasAction ? "cursor-pointer" : "px-3 py-3 align-middle", | ||
"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", | ||
Comment on lines
+229
to
+238
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Codebase verification Focus indicators need higher contrast for accessibility. The current focus indicator using 🔗 Analysis chainVerify focus indicator contrast. The custom focus indicators use subtle colors. Please ensure they meet WCAG contrast requirements for accessibility. 🏁 Scripts executedThe 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 |
||
className | ||
)} | ||
colSpan={colSpan} | ||
> | ||
{to ? ( | ||
<Link to={to} className={cn("focus-custom", flexClasses, actionClassName)}> | ||
<Link | ||
to={to} | ||
className={cn("cursor-pointer focus:outline-none", flexClasses, actionClassName)} | ||
tabIndex={isTabbableCell ? 0 : -1} | ||
> | ||
{children} | ||
</Link> | ||
) : onClick ? ( | ||
<button onClick={onClick} className={cn("focus-custom", flexClasses, actionClassName)}> | ||
<button | ||
onClick={onClick} | ||
className={cn("cursor-pointer focus:outline-none", flexClasses, actionClassName)} | ||
tabIndex={isTabbableCell ? 0 : -1} | ||
> | ||
{children} | ||
</button> | ||
) : ( | ||
|
@@ -258,7 +294,7 @@ export const TableCellChevron = forwardRef< | |
|
||
export const TableCellMenu = forwardRef< | ||
HTMLTableCellElement, | ||
{ | ||
Omit<TableCellProps, "variant"> & { | ||
className?: string; | ||
isSticky?: boolean; | ||
onClick?: (event: React.MouseEvent<HTMLButtonElement, MouseEvent>) => void; | ||
|
@@ -267,6 +303,7 @@ export const TableCellMenu = forwardRef< | |
popoverContent?: ReactNode; | ||
children?: ReactNode; | ||
isSelected?: boolean; | ||
variant?: TableVariant; | ||
} | ||
>( | ||
( | ||
|
@@ -283,6 +320,7 @@ export const TableCellMenu = forwardRef< | |
ref | ||
) => { | ||
const [isOpen, setIsOpen] = useState(false); | ||
const { variant } = useContext(TableContext); | ||
|
||
return ( | ||
<TableCell | ||
|
@@ -297,15 +335,18 @@ export const TableCellMenu = forwardRef< | |
<div className="relative h-full p-1"> | ||
<div | ||
className={cn( | ||
"absolute right-0 top-1/2 mr-1 flex -translate-y-1/2 items-center justify-end gap-0.5 rounded-[0.25rem] bg-background-dimmed p-0.5 group-hover/table-row:bg-background-bright group-hover/table-row:ring-1 group-hover/table-row:ring-grid-bright", | ||
isSelected && isSelectedStyle, | ||
isSelected && | ||
"group-hover/table-row:bg-charcoal-750 group-hover/table-row:ring-charcoal-600/50" | ||
"absolute right-0 top-1/2 mr-1 flex -translate-y-1/2 items-center justify-end gap-0.5 rounded-[0.25rem] p-0.5 group-hover/table-row:ring-1", | ||
variants[variant].menuButton | ||
)} | ||
> | ||
{/* Hidden buttons that show on hover */} | ||
{hiddenButtons && ( | ||
<div className="hidden pr-0.5 group-hover/table-row:block group-hover/table-row:border-r group-hover/table-row:border-grid-dimmed"> | ||
<div | ||
className={cn( | ||
"hidden pr-0.5 group-hover/table-row:block group-hover/table-row:border-r", | ||
variants[variant].menuButtonDivider | ||
)} | ||
> | ||
{hiddenButtons} | ||
</div> | ||
)} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this still need to be here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).