Add stacked toast notification component - #1359
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cd9d1129d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| add_init_fn(sqlpage_card); | ||
| add_init_fn(sqlpage_form); | ||
| add_init_fn(load_scripts); | ||
| add_init_fn(sqlpage_toast); |
There was a problem hiding this comment.
On an initial page load, this initializer cannot run until the deferred sqlpage.js executes after the entire streamed document has been parsed. Consequently, when a toast row precedes a slow query, Bootstrap leaves it hidden and its dismissal timer does not start until the full response finishes, so it cannot notify the user while later statements are running. Observe newly parsed toast nodes or otherwise initialize them before end-of-stream.
AGENTS.md reference: AGENTS.md:L36-L42
Useful? React with 👍 / 👎.
| container.classList.add( | ||
| "toast-container", | ||
| "position-fixed", | ||
| "p-3", | ||
| ...toast_positions[position], |
There was a problem hiding this comment.
When a query returns enough persistent (duration = 0) toasts for one position, this fixed container grows beyond the viewport because it has neither a height bound nor overflow handling. Toasts and their close controls below the screen then cannot be reached by scrolling the page, since the container remains fixed; this is especially easy to hit on mobile. Cap the stack to the viewport and make it independently scrollable or otherwise bound the queue.
Useful? React with 👍 / 👎.
| {{~/if~}} | ||
| </div> | ||
| {{~#if dismissible~}} | ||
| <button type="button" class="btn-close{{#if color}} btn-close-white{{/if}} flex-shrink-0" data-bs-dismiss="toast" aria-label="Close notification"></button> |
There was a problem hiding this comment.
For every colored dismissible toast this forces a white close icon, while light Tabler colors such as yellow deliberately use a dark text-yellow-fg foreground. In that scenario the close control has very low contrast against the light background even though the surrounding text remains readable. Render the icon using the selected foreground color rather than assuming every colored background is dark.
Useful? React with 👍 / 👎.
| ); | ||
|
|
||
| const temporary = page.locator("#toast-short"); | ||
| await expect(temporary).toBeVisible(); |
There was a problem hiding this comment.
The toast-short timer starts during page initialization, but page.goto() waits for the load event and the official-site shell loads several external stylesheets, fonts, and deferred scripts; the many assertions before this line add further delay. On a slow or network-constrained CI worker, the 2000 ms toast can already have been hidden and removed before toBeVisible() runs, making this test flaky. Trigger a fresh timed toast inside the test or use a duration that cannot expire before the initial visibility assertion.
Useful? React with 👍 / 👎.
| "hidden.bs.toast", | ||
| () => { | ||
| toast.remove(); |
There was a problem hiding this comment.
When a keyboard user activates the manual close button, focus is still on that button when hidden.bs.toast fires, and this callback immediately removes the focused element. The browser then commonly falls back to the document body, losing the user's place in the page or toast stack. Before removing a manually dismissed toast, move focus to a predictable target such as the next visible toast or the surrounding page content.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
https://github.com/sqlpage/SQLPage/blob/20f13342362d1db74f16ac0db0ab122b9f1d1b1e/sqlpage.js#L402
When automatic, non-triggered toasts are hidden, removing the DOM node without calling the Bootstrap instance's dispose() leaves the element and instance registered in Bootstrap's internal component data map. Pages that render many toasts, especially through repeatedly loaded fragments, therefore retain every dismissed notification until navigation even though the nodes are no longer visible; dispose the instance before removing the element.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const hash = normalize_hash(window.location.hash); | ||
| if (!hash) return; | ||
| for (const toast of document.querySelectorAll("[data-toast-trigger]")) { | ||
| if (normalize_hash(toast.dataset.toastTrigger) === hash) { |
There was a problem hiding this comment.
When a trigger contains spaces or non-ASCII characters, such as trigger = 'café' with a link to #café, browsers expose window.location.hash in percent-encoded form (#caf%C3%A9) while data-toast-trigger remains decoded. This comparison therefore never matches and the documented fragment-triggered toast does not open; normalize both sides to the same encoded or decoded representation.
Useful? React with 👍 / 👎.
| } | ||
| return; | ||
| } | ||
| toast.remove(); |
There was a problem hiding this comment.
When automatic, non-triggered toasts are hidden, removing the DOM node without calling the Bootstrap instance's dispose() leaves the element and instance registered in Bootstrap's internal component data map. Pages that render many toasts, especially through repeatedly loaded fragments, therefore retain every dismissed notification until navigation even though the nodes are no longer visible; dispose the instance before removing the element.
Useful? React with 👍 / 👎.
1 participant
Motivation
Description
sqlpage/templates/toast.handlebarsthat renders a.toastelement withdata-pre-init="toast", supportstitle,description,description_md(Markdown takes precedence),icon,color,dismissible,duration,position,id, andclass, and usesicon_img/markdownhelpers and accessible attributes (role="status",aria-live="polite").sqlpage_toastinsqlpage/sqlpage.jsthat normalizes position, creates/uses position-fixed.toast-containerper placement, moves toasts into the shared container, normalizesduration, instantiates the framework toast (window.bootstrap || window.tabler.bootstrap), shows toasts, and removes generated containers when empty.examples/official-site/sqlpage/migrations/76_toast.sqlinserting thetoastcomponent, parameters, defaults, and multiple executable examples demonstrating stacking, Markdown, persistence, and alternate placement.tests/sql_test_files/component_rendering/toast.sqland a Playwright interaction spec addition intests/end-to-end/official-site.spec.tsto validate visibility, stacking, timed/persistent behavior, dismissal, escaped vs. Markdown rendering, and placement, and updateCHANGELOG.mdaccordingly.Testing
npm run formatcompleted successfully and formatted JS files as expected.sqlite3ran and validated the new migration, confirming component/parameter/example rows and JSON validity for examples.npx biome check/npm testwere executed for the modified frontend files; the checks completed but the repository-wide Biome run surfaced unrelated pre-existing diagnostics (missingparseIntradix instances and several style warnings) that are outside the scope of this change.cargo testwas attempted but could not complete in this environment because building nativelibsqlite3-syshit a Rust toolchain limitation (cfg_selectunstable feature); Playwright end-to-end tests were not executed because the official-site server could not be started due to that build issue.Codex Task