max / goingson
- Co-Authored-By
- Claude Opus 5 (1M context) <noreply@anthropic.com>
4 files changed,
+114 insertions,
-43 deletions
| @@ -14,16 +14,23 @@ | |||
| 14 | 14 | /// hide mobile columns with `nth-child(n+5)` against a seven-column table plus a | |
| 15 | 15 | /// separate `nth-child(3)`, so inserting a column anywhere left of the cut hid | |
| 16 | 16 | /// the wrong one and nothing said so. | |
| 17 | + | /// | |
| 18 | + | /// `sorted` is `None` on every column here and always will be. Which way the | |
| 19 | + | /// table is ordered is a runtime fact that changes when a header is pressed, so | |
| 20 | + | /// a build script cannot hold it; `tasks-filter.js` writes it onto the heading | |
| 21 | + | /// as `aria-sort`, which is the same field said in the webview's idiom. | |
| 17 | 22 | const TASK_COLUMNS: &[Column<'static>] = &[ | |
| 18 | 23 | // Without it the row does not identify itself. | |
| 19 | 24 | Column { | |
| 20 | 25 | width: Width::Fill, | |
| 21 | 26 | priority: Priority::Essential, | |
| 27 | + | sortable: true, | |
| 22 | 28 | ..Column::new("description") | |
| 23 | 29 | }, | |
| 24 | 30 | Column { | |
| 25 | 31 | width: Width::Fixed, | |
| 26 | 32 | priority: Priority::Secondary, | |
| 33 | + | sortable: true, | |
| 27 | 34 | ..Column::new("project") | |
| 28 | 35 | }, | |
| 29 | 36 | // A single letter, and the row already carries its priority in the left | |
| @@ -31,11 +38,13 @@ | |||
| 31 | 38 | Column { | |
| 32 | 39 | width: Width::Fixed, | |
| 33 | 40 | priority: Priority::Optional, | |
| 41 | + | sortable: true, | |
| 34 | 42 | ..Column::new("priority") | |
| 35 | 43 | }, | |
| 36 | 44 | Column { | |
| 37 | 45 | width: Width::Fixed, | |
| 38 | 46 | priority: Priority::Secondary, | |
| 47 | + | sortable: true, | |
| 39 | 48 | ..Column::new("due") | |
| 40 | 49 | }, | |
| 41 | 50 | Column { | |
| @@ -180,9 +189,19 @@ | |||
| 180 | 189 | .map(|c| format!("\"{}\"", c.name)) | |
| 181 | 190 | .collect::<Vec<_>>() | |
| 182 | 191 | .join(", "); | |
| 192 | + | // Which of them offer a reorder, so the one place that says so is the | |
| 193 | + | // description. `check_sortable_headers` reads this same field out of | |
| 194 | + | // the const; this carries it to a reader that is not Rust. | |
| 195 | + | let sortable = columns | |
| 196 | + | .iter() | |
| 197 | + | .filter(|c| c.sortable) | |
| 198 | + | .map(|c| format!("\"{}\"", c.name)) | |
| 199 | + | .collect::<Vec<_>>() | |
| 200 | + | .join(", "); | |
| 183 | 201 | let _ = writeln!( | |
| 184 | 202 | json, | |
| 185 | - | " \"{table}\": {{ \"header\": \"{header}\", \"columns\": [{names}] }}{}", | |
| 203 | + | " \"{table}\": {{ \"header\": \"{header}\", \"columns\": [{names}], \ | |
| 204 | + | \"sortable\": [{sortable}] }}{}", | |
| 186 | 205 | if i + 1 == TABLES.len() { "" } else { "," } | |
| 187 | 206 | ); | |
| 188 | 207 | } | |
| @@ -341,6 +360,57 @@ | |||
| 341 | 360 | ) | |
| 342 | 361 | } | |
| 343 | 362 | ||
| 363 | + | /// Check the task header's markup against [`TASK_COLUMNS`]. | |
| 364 | + | /// | |
| 365 | + | /// The generated caret matches `.table-heading[data-sortable]` and | |
| 366 | + | /// `.table-heading[aria-sort="..."]`, and index.html is hand-written, so the | |
| 367 | + | /// description saying a column reorders buys nothing unless the heading says so | |
| 368 | + | /// too. That gap is what the whole defect was: `build.rs` had `sortable` false | |
| 369 | + | /// on all seven columns of a table that sorts, the headings carried a local | |
| 370 | + | /// `.sortable` class instead of `.table-heading`, and the generated rules sat | |
| 371 | + | /// in `layout.css` matching nothing while a hand-written copy drew the arrow. | |
| 372 | + | /// | |
| 373 | + | /// Both directions, because both drift. A sortable column whose heading lost | |
| 374 | + | /// `data-sortable` gets no cursor and no caret; a heading that grew one against | |
| 375 | + | /// a column the sort code cannot order offers a press that does nothing. | |
| 376 | + | /// | |
| 377 | + | /// Markup-shaped rather than parsed: this looks for `col-<name>` and | |
| 378 | + | /// `data-sortable` inside one heading `<div>`, which is what the file actually | |
| 379 | + | /// contains and all a build script should claim to understand. | |
| 380 | + | fn check_sortable_headers(frontend: &Path) { | |
| 381 | + | let html = fs::read_to_string(frontend.join("index.html")).expect("read index.html"); | |
| 382 | + | let headings: Vec<&str> = html | |
| 383 | + | .lines() | |
| 384 | + | .filter(|line| line.contains("role=\"columnheader\"") && line.contains("task-cell")) | |
| 385 | + | .collect(); | |
| 386 | + | ||
| 387 | + | for column in TASK_COLUMNS { | |
| 388 | + | let marker = format!("col-{}", column.name); | |
| 389 | + | let Some(heading) = headings.iter().find(|line| { | |
| 390 | + | line.contains(&format!("{marker} ")) || line.contains(&format!("{marker}\"")) | |
| 391 | + | }) else { | |
| 392 | + | // Only the sortable ones are required to be found. A column with no | |
| 393 | + | // heading of its own is the events table's problem, not this check's. | |
| 394 | + | assert!( | |
| 395 | + | !column.sortable, | |
| 396 | + | "TASK_COLUMNS says `{}` is sortable, but index.html has no \ | |
| 397 | + | `{marker}` column header to carry `data-sortable`", | |
| 398 | + | column.name | |
| 399 | + | ); | |
| 400 | + | continue; | |
| 401 | + | }; | |
| 402 | + | let marked = heading.contains("data-sortable"); | |
| 403 | + | assert_eq!( | |
| 404 | + | marked, column.sortable, | |
| 405 | + | "`{}`: TASK_COLUMNS says sortable = {}, index.html says {marked}. \ | |
| 406 | + | The description and the markup are the same fact; change both.", | |
| 407 | + | column.name, column.sortable | |
| 408 | + | ); | |
| 409 | + | } | |
| 410 | + | ||
| 411 | + | println!("cargo:rerun-if-changed=frontend/index.html"); | |
| 412 | + | } | |
| 413 | + | ||
| 344 | 414 | /// Widths that are tuning inside the wide shell, not a shell boundary. | |
| 345 | 415 | /// | |
| 346 | 416 | /// A shell boundary is a [`SizeClass`] edge and belongs to makeover-geometry. | |
| @@ -377,6 +447,7 @@ | |||
| 377 | 447 | // touch string is makeover-geometry's and no app gets a say. | |
| 378 | 448 | makeover_build::check_breakpoints(&frontend, TUNING_WIDTHS); | |
| 379 | 449 | makeover_build::check_touch_density(frontend.join("js")); | |
| 450 | + | check_sortable_headers(&frontend); | |
| 380 | 451 | ||
| 381 | 452 | println!("cargo:rerun-if-changed=build.rs"); | |
| 382 | 453 |
| @@ -173,18 +173,10 @@ | |||
| 173 | 173 | </div> | |
| 174 | 174 | <div class="raised task-table" id="task-table" role="grid" aria-label="Tasks list"> | |
| 175 | 175 | <div class="task-header-row" role="row"> | |
| 176 | - | <div class="task-cell col-description sortable" data-sort="description" data-act="tasks.sort" data-a1="description" role="columnheader" tabindex="0"> | |
| 177 | - | Description <span class="sort-arrow"></span> | |
| 178 | - | </div> | |
| 179 | - | <div class="task-cell col-project sortable" data-sort="project" data-act="tasks.sort" data-a1="project" role="columnheader" tabindex="0"> | |
| 180 | - | Project <span class="sort-arrow"></span> | |
| 181 | - | </div> | |
| 182 | - | <div class="task-cell col-priority sortable" data-sort="priority" data-act="tasks.sort" data-a1="priority" role="columnheader" tabindex="0" aria-label="Priority"> | |
| 183 | - | Priority <span class="sort-arrow"></span> | |
| 184 | - | </div> | |
| 185 | - | <div class="task-cell col-due sortable" data-sort="due" data-act="tasks.sort" data-a1="due" role="columnheader" tabindex="0"> | |
| 186 | - | Due <span class="sort-arrow"></span> | |
| 187 | - | </div> | |
| 176 | + | <div class="table-heading task-cell col-description" data-sortable data-sort="description" data-act="tasks.sort" data-a1="description" role="columnheader" tabindex="0">Description</div> | |
| 177 | + | <div class="table-heading task-cell col-project" data-sortable data-sort="project" data-act="tasks.sort" data-a1="project" role="columnheader" tabindex="0">Project</div> | |
| 178 | + | <div class="table-heading task-cell col-priority" data-sortable data-sort="priority" data-act="tasks.sort" data-a1="priority" role="columnheader" tabindex="0" aria-label="Priority">Priority</div> | |
| 179 | + | <div class="table-heading task-cell col-due" data-sortable data-sort="due" data-act="tasks.sort" data-a1="due" role="columnheader" tabindex="0">Due</div> | |
| 188 | 180 | <div class="task-cell col-recurrence" role="columnheader">Recurs</div> | |
| 189 | 181 | <div class="task-cell col-progress" role="columnheader">Progress</div> | |
| 190 | 182 | <div class="task-cell col-actions task-actions-header" role="columnheader"><span class="sr-only">Actions</span></div> |
| @@ -1236,39 +1236,46 @@ | |||
| 1236 | 1236 | background: var(--surface-overlay); | |
| 1237 | 1237 | } | |
| 1238 | 1238 | ||
| 1239 | - | /* Sortable column headers */ | |
| 1240 | - | .sortable { | |
| 1241 | - | cursor: pointer; | |
| 1239 | + | /* Sortable column headers | |
| 1240 | + | ||
| 1241 | + | The cursor and the caret are NOT here. `.table-heading[data-sortable]` and | |
| 1242 | + | `.table-heading[aria-sort="..."]::after` are generated into css/layout.css by | |
| 1243 | + | makeover-webview out of `Column::sortable` and `Column::sorted`, and this file | |
| 1244 | + | used to carry a hand-written second copy keyed on `.sort-asc`/`.sort-desc`. | |
| 1245 | + | Two channels, one state: `tasks-filter.js` wrote both and only the local one | |
| 1246 | + | was ever visible, because the markup never carried `.table-heading` for the | |
| 1247 | + | generated pair to match. | |
| 1248 | + | ||
| 1249 | + | What is left below is what the design system does not answer -- whether a | |
| 1250 | + | pressable heading takes a hover surface, and the caret's spacing -- and | |
| 1251 | + | nothing that restates the glyph. */ | |
| 1252 | + | .table-heading[data-sortable] { | |
| 1242 | 1253 | user-select: none; | |
| 1243 | 1254 | white-space: nowrap; | |
| 1244 | 1255 | } | |
| 1245 | 1256 | ||
| 1246 | - | .sortable:hover { | |
| 1257 | + | .table-heading[data-sortable]:hover { | |
| 1247 | 1258 | background: var(--hover-surface); | |
| 1248 | 1259 | } | |
| 1249 | 1260 | ||
| 1250 | - | .sort-arrow { | |
| 1261 | + | /* makeover-tui and makeover-immediate both emit their caret with a leading | |
| 1262 | + | space; the webview emitter appends the bare glyph, so the gap is the app's to | |
| 1263 | + | put back. Reserved on every sortable heading rather than only the sorted one, | |
| 1264 | + | or pressing a header shifts the row it sits in. | |
| 1265 | + | ||
| 1266 | + | Box only, never `content`, on the sorted arm. This layer outranks `makeover`, | |
| 1267 | + | so declaring `content` here at all would win over the generated caret and put | |
| 1268 | + | the arrow back to nothing -- which is the same override this block was | |
| 1269 | + | written to delete, one property along. The empty box goes on the arm the | |
| 1270 | + | generator does not match. */ | |
| 1271 | + | .table-heading[data-sortable]:not([aria-sort])::after { | |
| 1272 | + | content: ''; | |
| 1273 | + | } | |
| 1274 | + | ||
| 1275 | + | .table-heading[data-sortable]::after { | |
| 1251 | 1276 | display: inline-block; | |
| 1252 | - | width: 0.8em; | |
| 1277 | + | min-width: 1ch; | |
| 1253 | 1278 | margin-left: var(--gap-bound); | |
| 1254 | - | opacity: 0.3; | |
| 1255 | - | } | |
| 1256 | - | ||
| 1257 | - | .sort-arrow::after { | |
| 1258 | - | content: '\2195'; /* Up-down arrow */ | |
| 1259 | - | } | |
| 1260 | - | ||
| 1261 | - | .sortable.sort-asc .sort-arrow::after { | |
| 1262 | - | content: '\2191'; /* Up arrow */ | |
| 1263 | - | } | |
| 1264 | - | ||
| 1265 | - | .sortable.sort-desc .sort-arrow::after { | |
| 1266 | - | content: '\2193'; /* Down arrow */ | |
| 1267 | - | } | |
| 1268 | - | ||
| 1269 | - | .sortable.sort-asc .sort-arrow, | |
| 1270 | - | .sortable.sort-desc .sort-arrow { | |
| 1271 | - | opacity: 1; | |
| 1272 | 1279 | } | |
| 1273 | 1280 | ||
| 1274 | 1281 | /* Overdue task highlighting */ |
| @@ -180,17 +180,18 @@ | |||
| 180 | 180 | GoingsOn.tasks.load(); | |
| 181 | 181 | } | |
| 182 | 182 | ||
| 183 | + | // One channel, and it is `aria-sort`. The caret comes from the generated | |
| 184 | + | // `.table-heading[aria-sort]::after` in css/layout.css, so the arrow a | |
| 185 | + | // sighted user sees and the order a screen reader announces are the same | |
| 186 | + | // attribute rather than two that can disagree. | |
| 183 | 187 | function updateSortArrows() { | |
| 184 | - | document.querySelectorAll('#task-table .sortable').forEach(th => { | |
| 185 | - | th.classList.remove('sort-asc', 'sort-desc'); | |
| 188 | + | document.querySelectorAll('#task-table [data-sortable]').forEach(th => { | |
| 186 | 189 | th.removeAttribute('aria-sort'); | |
| 187 | 190 | }); | |
| 188 | 191 | ||
| 189 | - | const currentHeader = document.querySelector(`#task-table .sortable[data-sort="${currentSortColumn}"]`); | |
| 192 | + | const currentHeader = document.querySelector(`#task-table [data-sortable][data-sort="${currentSortColumn}"]`); | |
| 190 | 193 | if (currentHeader) { | |
| 191 | - | const isAsc = currentSortDirection === 'asc'; | |
| 192 | - | currentHeader.classList.add(isAsc ? 'sort-asc' : 'sort-desc'); | |
| 193 | - | currentHeader.setAttribute('aria-sort', isAsc ? 'ascending' : 'descending'); | |
| 194 | + | currentHeader.setAttribute('aria-sort', currentSortDirection === 'asc' ? 'ascending' : 'descending'); | |
| 194 | 195 | } | |
| 195 | 196 | } | |
| 196 | 197 |