diff --git a/app/cypress/support/component.js b/app/cypress/support/component.js index c9feffc5..9cacf352 100644 --- a/app/cypress/support/component.js +++ b/app/cypress/support/component.js @@ -22,6 +22,12 @@ import '@mantine/core/styles.css'; // App.js loads this too. Without it a Notification renders unstyled here, so a toast had no // surface and no title colour of Mantine's -- and its contrast test could not fail. import '@mantine/notifications/styles.css'; +/* + * App.js loads this as well. Rules for things outside the component library live here -- + * the nav bar, the file browser rows -- and without it a spec covering one of them measures + * a component the app never renders. + */ +import '../../src/App.css'; // react-router v7 ships these directly; `react-router-dom` is not installed. import {MemoryRouter, Route, Routes} from "react-router"; import {QueryProvider} from "../../src/hooks/customHooks"; diff --git a/app/src/App.css b/app/src/App.css index 0d741fa4..8f9924d7 100644 --- a/app/src/App.css +++ b/app/src/App.css @@ -215,6 +215,21 @@ td.file-path { cursor: pointer; } +/* + * The row is an icon followed by a filename, as inline siblings. An inline SVG + * sits on the text baseline by default, so a 24px glyph next to 13px text hangs + * well above it and the name looks like it is resting on the icon's floor. + * `middle` centres the glyph against the text instead. + * + * The margin is the gap that was never there: the folder icon touched the first + * letter of its own name, and two icons (a folder and its ignored marker) touched + * each other. + */ +td.file-path > svg { + vertical-align: middle; + margin-right: 0.35em; +} + /* Necessary until react-semantic-toasts is upgraded to Node 18 */ .ui-alerts { position: fixed; diff --git a/app/src/components/ui/DataTable.tsx b/app/src/components/ui/DataTable.tsx index 52532d98..c305eb43 100644 --- a/app/src/components/ui/DataTable.tsx +++ b/app/src/components/ui/DataTable.tsx @@ -5,9 +5,15 @@ import {Icon} from './Icon'; /* * Tables. * - * Borders do structural work here: full 1px cell borders, a `--head` header row, - * and zebra striping. Wide tables scroll in their own container so the page - * body never scrolls sideways. + * Rows are separated by a horizontal rule and nothing else. There are no column + * borders: a full grid draws a line around every cell, which is a lot of ink to + * say something the column headings already say, and it made the header row -- + * which had no surface of its own -- read as one more cell in the grid. The + * header now carries `--head` and a full-strength rule under it, and the rules + * between body rows are `--table-line`, deliberately fainter. See ui.css. + * + * Wide tables scroll in their own container so the page body never scrolls + * sideways. * * The compound names mirror Semantic's (Header/Body/Row/Cell/HeaderCell/Footer) * so migrating a call site is a rename, not a rewrite. @@ -21,7 +27,6 @@ export interface TableProps extends MTableProps { function TableBase({scrollable = true, ...props}: TableProps) { const table = { }); }); +/* WCAG contrast between two rgb() strings, for judging how loud a border is. */ +const luminance = (colour) => { + const [r, g, b] = (colour.match(/[\d.]+/g) || []).map(Number); + const channel = (v) => { + const s = v / 255; + return s <= 0.03928 ? s / 12.92 : Math.pow((s + 0.055) / 1.055, 2.4); + }; + return 0.2126 * channel(r) + 0.7152 * channel(g) + 0.0722 * channel(b); +}; + +const contrast = (a, b) => { + const [x, y] = [luminance(a), luminance(b)].sort((p, q) => q - p); + return (x + 0.05) / (y + 0.05); +}; + +const sampleTable = + + + URL + Size + + + + one1 kB + two2 kB + three3 kB + +
; + +describe('a table header is a surface, not just bold text', () => { + themeNames.forEach((theme) => { + it(`separates the header from every body row in ${theme}`, () => { + /* + * Mantine gives thead no background, so the header painted whatever was behind + * the table -- which is exactly what an unstriped body row paints. Against a + * striped body the header read as one more stripe, and weight was the only + * thing telling them apart. None of this is visible to jsdom: the colours are + * tokens, and "what is behind an unstriped row" is a question about painting. + */ + cy.mountUI(sampleTable, {theme}); + + cy.get('thead th').first().should(($th) => { + const header = getComputedStyle($th[0]).backgroundColor; + expect(header, 'the header paints a surface of its own') + .to.not.match(/^rgba\(.*,\s*0(\.0+)?\)$/); + + const rows = [...Cypress.$('tbody tr')].map(row => { + // An unstriped row paints nothing, so what shows is the page behind it. + const own = getComputedStyle(row).backgroundColor; + return /^rgba\(.*,\s*0(\.0+)?\)$/.test(own) + ? getComputedStyle(document.documentElement).backgroundColor + : own; + }); + expect(rows.length).to.be.greaterThan(2); + rows.forEach((row, index) => { + expect(header, `header vs body row ${index}`).to.not.equal(row); + }); + }); + }); + }); + + it('rules off the header with a heavier line than it puts between rows', () => { + /* + * The header/body division is the one that carries meaning -- everything above it + * names, everything below is data -- so it is the one line allowed to be loud. + */ + cy.mountUI(sampleTable, {theme: 'light'}); + + cy.get('thead th').first().should(($th) => { + const header = getComputedStyle($th[0]); + const row = getComputedStyle(Cypress.$('tbody tr')[0]); + expect(parseFloat(header.borderBottomWidth), 'header rule width') + .to.be.greaterThan(parseFloat(row.borderBottomWidth)); + }); + }); +}); + +describe('table rules', () => { + themeNames.forEach((theme) => { + it(`keeps the rule between rows quieter than the table outline in ${theme}`, () => { + /* + * With a rule under every row at full border strength, a long table reads as a + * stack of boxes rather than a list. `--table-line` is the quieter token; the + * outline around the table is not a row rule and keeps `--border`. + * + * Measured as contrast against the row's own surface rather than compared to a + * token, so the claim is about what the eye gets. + */ + cy.mountUI(sampleTable, {theme}); + + cy.get('tbody tr').first().should(($row) => { + const table = Cypress.$('table')[0]; + const token = (name) => hexToRgb(getComputedStyle(document.documentElement) + .getPropertyValue(name).trim()); + /* + * Against `--panel`, the surface a row paints on. Not against the root + * background: nothing paints under a mounted component, so it reads + * `rgba(0, 0, 0, 0)`, and measuring both lines against black ranks the + * *paler* one as louder -- which is how this assertion first passed + * backwards. + */ + const surface = token('--panel'); + const rule = contrast(getComputedStyle($row[0]).borderBottomColor, surface); + const outline = contrast(getComputedStyle(table).borderTopColor, surface); + + expect(rule, 'row rule is quieter than the table outline').to.be.lessThan(outline); + }); + }); + + it(`draws nothing between the cells of a row in ${theme}`, () => { + cy.mountUI(sampleTable, {theme}); + + cy.get('tbody tr').first().find('td').first().should(($cell) => { + expect(parseFloat(getComputedStyle($cell[0]).borderRightWidth), + 'no rule between columns').to.equal(0); + }); + }); + }); +}); + +describe('a file row lines its icon up with its name', () => { + /* + * The file browser writes an icon and a filename as inline siblings in one cell. An + * inline SVG sits on the text baseline, so a 24px glyph beside 13px text hangs well + * above it -- the icon looked centred in the row and the name looked like it was + * resting on the icon's floor. + * + * Purely a question of layout: jsdom returns zeros from getBoundingClientRect, so + * nothing there can tell a centred glyph from one hanging off a baseline. + */ + const fileRow = + + + + + big_buck_bunny.mp3 + + + +
; + + it('centres the glyph against the text rather than sitting it on the baseline', () => { + cy.mountUI(fileRow, {theme: 'light'}); + + cy.get('td.file-path svg').should(($icon) => { + const icon = $icon[0].getBoundingClientRect(); + // The text node's own box, which is what the eye compares the icon to. + const range = document.createRange(); + range.selectNodeContents($icon[0].nextSibling); + const text = range.getBoundingClientRect(); + + const iconCentre = icon.top + icon.height / 2; + const textCentre = text.top + text.height / 2; + expect(Math.abs(iconCentre - textCentre), 'icon centre vs text centre') + .to.be.at.most(2); + }); + }); + + it('leaves a gap between the glyph and the name', () => { + // They touched: the folder icon ran into the first letter of its own folder name. + cy.mountUI(fileRow, {theme: 'light'}); + + cy.get('td.file-path svg').should(($icon) => { + const range = document.createRange(); + range.selectNodeContents($icon[0].nextSibling); + const gap = range.getBoundingClientRect().left - $icon[0].getBoundingClientRect().right; + expect(gap, 'gap between icon and filename').to.be.greaterThan(2); + }); + }); +}); + describe('StatisticGroup separates its statistics with nothing but space', () => { /* * The group has no frame, no rules between cells and no surface of its own: the diff --git a/app/src/components/ui/ui.css b/app/src/components/ui/ui.css index 2be019d8..b3daef1e 100644 --- a/app/src/components/ui/ui.css +++ b/app/src/components/ui/ui.css @@ -1183,6 +1183,51 @@ html[data-theme="night"] .wrolpi-status-failed { /* ----------------------------------------------------------------- Tables */ +/* + * The header is a surface, not just bold text. + * + * Mantine gives `thead` no background at all, so the header showed whatever was + * behind the table -- which is exactly what the unstriped body rows show. Bold + * type was the only thing separating the two, and against a striped body the + * header read as one more stripe. `--head` is the token for a recessed surface + * and is distinct from both the panel and the stripe in all four themes. + */ +.mantine-Table-thead .mantine-Table-th { + background: var(--head); +} + +/* + * A full-strength rule under the header, because that is the one division in a + * table that carries meaning: everything above it names, everything below it is + * data. The rules between body rows are `--table-line` and much fainter -- see + * the note there. Applied to the last header row so a grouped header does not + * get a hard rule through the middle of itself. + */ +.mantine-Table-thead .mantine-Table-tr:last-child .mantine-Table-th { + border-bottom: 2px solid var(--border); +} + +/* + * Row rules are quieter than any other border in the interface. They only have + * to let the eye track across a row; at `--border` strength, with a rule under + * every row, a long table turns into a stack of boxes. + * + * `html` for specificity, not decoration: Mantine declares this variable on its + * own hashed class, which ties with `.mantine-Table-table` and wins on source + * order because its stylesheet is imported after this one. + */ +html .mantine-Table-table { + --table-border-color: var(--table-line); +} + +/* + * The outline around the whole table is not a row rule and keeps full strength; + * Mantine draws both from the same variable, so it has to be restated. + */ +html .mantine-Table-table[data-with-table-border] { + border-color: var(--border); +} + /* A sortable header: a real button, so the column is reachable by keyboard. */ .wrolpi-th-sort { display: inline-flex; diff --git a/app/src/components/ui/ui.test.js b/app/src/components/ui/ui.test.js index b4e94aaf..4a84e0a6 100644 --- a/app/src/components/ui/ui.test.js +++ b/app/src/components/ui/ui.test.js @@ -737,6 +737,25 @@ describe('Table', () => { expect(screen.getByRole('cell', {name: 'example.com'})).toBeInTheDocument(); }); + it('draws no borders between columns', () => { + /* + * A full grid is a lot of ink to repeat what the column headings already say, and + * it cost the header its identity: with no surface of its own, a bordered header + * cell looked like one more cell in the grid. Mantine marks the choice on the + * table, so the absence is assertable without a browser. + */ + const {container} = renderUI( + + ab + +
); + + const table = container.querySelector('table'); + expect(table).not.toHaveAttribute('data-with-column-border'); + // The outline around the table is a different thing and stays. + expect(table).toHaveAttribute('data-with-table-border'); + }); + it('marks failed rows for the danger border', () => { const {container} = renderUI( diff --git a/app/src/themes/tokens.css b/app/src/themes/tokens.css index f16dda44..0e62035d 100644 --- a/app/src/themes/tokens.css +++ b/app/src/themes/tokens.css @@ -24,6 +24,8 @@ html[data-theme="light"] { --panel: #ffffff; --head: #e4e6e5; --border: #8f9698; + /* The rule between table rows: softer than --border, which reads as a grid. */ + --table-line: #eceeed; --text: #22282a; --muted: #5f686a; @@ -66,6 +68,8 @@ html[data-theme="dark"] { --panel: #26292b; --head: #141617; --border: #4a5052; + /* The rule between table rows: softer than --border, which reads as a grid. */ + --table-line: #343a3c; --text: #e6e8e6; --muted: #93999b; @@ -99,6 +103,8 @@ html[data-theme="night"] { --panel: #160404; --head: #000000; --border: #5c1010; + /* The rule between table rows: softer than --border, which reads as a grid. */ + --table-line: #3a0d0d; --text: #e04a4a; --muted: #7a2525; @@ -128,6 +134,8 @@ html[data-theme="amber"] { --panel: #1a0f00; --head: #000000; --border: #5c3a08; + /* The rule between table rows: softer than --border, which reads as a grid. */ + --table-line: #3d2708; --text: #ff9500; --muted: #8a5810;