-
Notifications
You must be signed in to change notification settings - Fork 22
Improved mini-table resource name truncation #3182
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
base: main
Are you sure you want to change the base?
Changes from all commits
9fc620d
3c126af
461fc51
7514019
6885f2a
148a9ac
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,3 +1,5 @@ | ||
| import { useRef, useState, type ReactNode, useMemo } from 'react' | ||
|
|
||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
|
|
@@ -12,6 +14,8 @@ import { classed } from '~/util/classed' | |
| import { Button } from './Button' | ||
| import { EmptyMessage } from './EmptyMessage' | ||
| import { Table as BigTable } from './Table' | ||
| import { textWidth } from './text-width' | ||
| import { Tooltip } from './Tooltip' | ||
|
|
||
| type Children = { children: React.ReactNode } | ||
|
|
||
|
|
@@ -29,10 +33,18 @@ const Body = classed.tbody`` | |
|
|
||
| const Row = classed.tr`*:border-default last:*:border-b *:first:border-l *:last:border-r` | ||
|
|
||
| const Cell = ({ children }: Children) => { | ||
| const Cell = ({ | ||
| children, | ||
| className, | ||
| style, | ||
| }: { | ||
| children: ReactNode | ||
| className?: string | ||
| style?: React.CSSProperties | ||
| }) => { | ||
| return ( | ||
| <td> | ||
| <div>{children}</div> | ||
| <td className={className} style={style}> | ||
| <div className="relative">{children}</div> | ||
| </td> | ||
| ) | ||
| } | ||
|
|
@@ -78,6 +90,36 @@ const RemoveCell = ({ onClick, label }: { onClick: () => void; label: string }) | |
| </Cell> | ||
| ) | ||
|
|
||
| const TruncateCell = ({ text }: { text: string }) => { | ||
| const ref = useRef<HTMLDivElement>(null) | ||
| const [isTruncated, setIsTruncated] = useState(false) | ||
|
|
||
| const inner = ( | ||
| <div | ||
| ref={ref} | ||
| className="absolute inset-x-3 truncate" | ||
| onMouseEnter={() => { | ||
| const el = ref.current | ||
| setIsTruncated(!!el && el.scrollWidth > el.clientWidth) | ||
| }} | ||
| > | ||
| {text} | ||
| </div> | ||
| ) | ||
|
|
||
| return ( | ||
| <div className="flex h-full w-full items-center justify-center"> | ||
| {isTruncated ? ( | ||
| <Tooltip content={text} placement="bottom"> | ||
| {inner} | ||
| </Tooltip> | ||
| ) : ( | ||
| inner | ||
| )} | ||
| </div> | ||
| ) | ||
| } | ||
|
|
||
| type ClearAndAddButtonsProps = { | ||
| addButtonCopy: string | ||
| disabled: boolean | ||
|
|
@@ -107,8 +149,14 @@ export const ClearAndAddButtons = ({ | |
|
|
||
| type Column<T> = { | ||
| header: string | ||
| cell: (item: T, index: number) => React.ReactNode | ||
| } | ||
| } & ( | ||
| | { cell: (item: T, index: number) => React.ReactNode } | ||
| | { | ||
| /** Columns with `text` auto-truncate and share remaining table width | ||
| * proportionally based on their measured text content. */ | ||
| text: (item: T) => string | ||
| } | ||
| ) | ||
|
|
||
| type MiniTableProps<T> = { | ||
| ariaLabel: string | ||
|
|
@@ -125,6 +173,66 @@ type MiniTableProps<T> = { | |
| className?: string | ||
| } | ||
|
|
||
| function isTextColumn<T>( | ||
| col: Column<T> | ||
| ): col is { header: string; text: (item: T) => string } { | ||
| return 'text' in col | ||
| } | ||
|
|
||
| /** | ||
| * For each text column, find the max text width across all items, then | ||
| * distribute remaining table width proportionally. Returns a per-column | ||
| * style object (undefined for fit-to-content columns). | ||
| */ | ||
| function useColumnWidths<T>(columns: Column<T>[], items: T[]) { | ||
|
Contributor
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. it prevents having to reason about the kinds of things coming out of this function if you just uniformly return "an object full of props". it's kind of easier to express the idea as a diff: diff --git a/app/ui/lib/MiniTable.tsx b/app/ui/lib/MiniTable.tsx
index 56dd76aa..0ff54366 100644
--- a/app/ui/lib/MiniTable.tsx
+++ b/app/ui/lib/MiniTable.tsx
@@ -184,12 +184,15 @@ function isTextColumn<T>(
* distribute remaining table width proportionally. Returns a per-column
* style object (undefined for fit-to-content columns).
*/
-function useColumnWidths<T>(columns: Column<T>[], items: T[]) {
+function useColumnWidths<T>(
+ columns: Column<T>[],
+ items: T[]
+): Pick<React.ComponentProps<'td'>, 'className' | 'style'>[] {
return useMemo(() => {
const hasTextCols = columns.some(isTextColumn)
if (!hasTextCols || items.length === 0) {
// Fall back to the old behavior: first column gets w-full
- return columns.map((_, i) => (i === 0 ? 'w-full' : undefined))
+ return columns.map((_, i) => (i === 0 ? { className: 'w-full' } : {}))
}
// Measure max natural text width per text column.
@@ -209,7 +212,7 @@ function useColumnWidths<T>(columns: Column<T>[], items: T[]) {
const textColCount = maxWidths.filter((w) => w > 0).length
const totalTextWidth = maxWidths.reduce((sum, w) => sum + w, 0)
if (totalTextWidth === 0 || textColCount === 0) {
- return columns.map((_, i) => (i === 0 ? 'w-full' : undefined))
+ return columns.map((_, i) => (i === 0 ? { className: 'w-full' } : {}))
}
// Max ratio between widest and narrowest text column.
@@ -226,9 +229,9 @@ function useColumnWidths<T>(columns: Column<T>[], items: T[]) {
// Text columns share available space proportionally; others fit content
return columns.map((col, i) => {
- if (!isTextColumn(col)) return undefined
+ if (!isTextColumn(col)) return {}
const pct = (clamped[i] / clampedTotal) * 100
- return { width: `${pct.toFixed(1)}%` } as const
+ return { style: { width: `${pct.toFixed(1)}%` } }
})
}, [columns, items])
}
@@ -263,11 +266,8 @@ export function MiniTable<T>({
items.map((item, index) => (
<Row tabIndex={0} aria-rowindex={index + 1} key={rowKey(item, index)}>
{columns.map((column, colIndex) => {
- const w = colWidths[colIndex]
- const className = typeof w === 'string' ? w : undefined
- const style = typeof w === 'object' ? w : undefined
return (
- <Cell key={colIndex} className={className} style={style}>
+ <Cell key={colIndex} {...colWidths[colIndex]}>
{isTextColumn(column) ? (
<TruncateCell text={column.text(item)} />
) : (
Contributor
Author
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. I like this |
||
| return useMemo(() => { | ||
| const hasTextCols = columns.some(isTextColumn) | ||
| if (!hasTextCols || items.length === 0) { | ||
| // Fall back to the old behavior: first column gets w-full | ||
| return columns.map((_, i) => (i === 0 ? 'w-full' : undefined)) | ||
| } | ||
|
|
||
| // Measure max natural text width per text column. | ||
| // text-sans-md = 400 14px/1.125rem SuisseIntl, letter-spacing 0.03rem | ||
| const font = '400 14px SuisseIntl' | ||
| const letterSpacing = '0.03rem' | ||
| const maxWidths = columns.map((col) => { | ||
| if (!isTextColumn(col)) return 0 | ||
| let max = 0 | ||
| for (const item of items) { | ||
| const w = textWidth(col.text(item), font, letterSpacing) | ||
| if (w > max) max = w | ||
| } | ||
| return max | ||
| }) | ||
|
|
||
| const textColCount = maxWidths.filter((w) => w > 0).length | ||
| const totalTextWidth = maxWidths.reduce((sum, w) => sum + w, 0) | ||
| if (totalTextWidth === 0 || textColCount === 0) { | ||
|
Contributor
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. trivial, but since you're not gonna find a negative width, if one of these is true the other is true, so only one needs checking |
||
| return columns.map((_, i) => (i === 0 ? 'w-full' : undefined)) | ||
| } | ||
|
|
||
| // Max ratio between widest and narrowest text column. | ||
| // 1 = all equal, higher = more variation. | ||
| const maxWidthRatio = 5 / 2 | ||
| const equalShare = totalTextWidth / textColCount | ||
| const spread = Math.sqrt(maxWidthRatio) | ||
| const floor = equalShare / spread | ||
| const ceiling = equalShare * spread | ||
| const clamped = maxWidths.map((w) => | ||
| w > 0 ? Math.min(Math.max(w, floor), ceiling) : 0 | ||
|
Contributor
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. there are some convenience functions from remeda you can use here, |
||
| ) | ||
| const clampedTotal = clamped.reduce((sum, w) => sum + w, 0) | ||
|
|
||
| // Text columns share available space proportionally; others fit content | ||
| return columns.map((col, i) => { | ||
| if (!isTextColumn(col)) return undefined | ||
| const pct = (clamped[i] / clampedTotal) * 100 | ||
| return { width: `${pct.toFixed(1)}%` } as const | ||
| }) | ||
| }, [columns, items]) | ||
| } | ||
|
|
||
| /** If `emptyState` is left out, `MiniTable` renders null when `items` is empty. */ | ||
| export function MiniTable<T>({ | ||
| ariaLabel, | ||
|
|
@@ -136,6 +244,8 @@ export function MiniTable<T>({ | |
| emptyState, | ||
| className, | ||
| }: MiniTableProps<T>) { | ||
| const colWidths = useColumnWidths(columns, items) | ||
|
|
||
| if (!emptyState && items.length === 0) return null | ||
|
|
||
| return ( | ||
|
|
@@ -152,9 +262,20 @@ export function MiniTable<T>({ | |
| {items.length ? ( | ||
| items.map((item, index) => ( | ||
| <Row tabIndex={0} aria-rowindex={index + 1} key={rowKey(item, index)}> | ||
| {columns.map((column, colIndex) => ( | ||
| <Cell key={colIndex}>{column.cell(item, index)}</Cell> | ||
| ))} | ||
| {columns.map((column, colIndex) => { | ||
| const w = colWidths[colIndex] | ||
| const className = typeof w === 'string' ? w : undefined | ||
| const style = typeof w === 'object' ? w : undefined | ||
| return ( | ||
| <Cell key={colIndex} className={className} style={style}> | ||
| {isTextColumn(column) ? ( | ||
| <TruncateCell text={column.text(item)} /> | ||
| ) : ( | ||
| column.cell(item, index) | ||
| )} | ||
| </Cell> | ||
| ) | ||
| })} | ||
|
|
||
| <RemoveCell | ||
| onClick={() => onRemoveItem(item)} | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| /* | ||
| * This Source Code Form is subject to the terms of the Mozilla Public | ||
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, you can obtain one at https://mozilla.org/MPL/2.0/. | ||
| * | ||
| * Copyright Oxide Computer Company | ||
| */ | ||
|
|
||
| let ctx: CanvasRenderingContext2D | null = null | ||
|
|
||
| function getContext(): CanvasRenderingContext2D { | ||
| if (!ctx) { | ||
| const canvas = document.createElement('canvas') | ||
| // eslint-disable-next-line @typescript-eslint/no-non-null-assertion -- offscreen canvas always has 2d context | ||
| ctx = canvas.getContext('2d')! | ||
| } | ||
| return ctx | ||
| } | ||
|
|
||
| const cache = new Map<string, number>() | ||
|
|
||
| /** | ||
| * Measure the rendered pixel width of `text` using Canvas `measureText`. | ||
|
Contributor
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. this is relatively quick, though you could certainly give up a meaningful chunk of the render cycle calculating a sufficiently large table (i mean on the order of, say, thousands of unique cells). realistically i'm not sure that's a reachable scale, but given the constraints on names (which are most of these columns), i'd wager that
Contributor
Author
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. In this specific use case ... perhaps. Though it depends very much on the character choice and combinations. For columns as narrow as these get that effect can be quite pronounced. I mention it briefly here: #3182 (comment) Would also say there's value in consistency beyond this use case. That's to say, rather than adding an exemption here and using string length, it's probably clearer to use text measurement everywhere (which is my recommendation for the Mini-tables are mini, so we can safely say we're not going to be calculating significant numbers of cells. |
||
| * Accounts for font shaping, kerning, and letter-spacing. Reuses a single | ||
| * offscreen canvas context and caches results. | ||
| */ | ||
| export function textWidth(text: string, font: string, letterSpacing = '0px'): number { | ||
| const key = font + '\0' + letterSpacing + '\0' + text | ||
| const cached = cache.get(key) | ||
| if (cached != null) return cached | ||
|
|
||
| const context = getContext() | ||
| context.font = font | ||
| context.letterSpacing = letterSpacing | ||
| const width = context.measureText(text).width | ||
| cache.set(key, width) | ||
| return width | ||
| } | ||
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.
I might be missing something, but I don't believe we're using this
index, so it can be dropped here and at the callsite (column.cell(item, index)on line 274).