Skip to content

Conversation

@myieye
Copy link
Collaborator

@myieye myieye commented Oct 6, 2025

And keep the close button in view. Using sticky is a bit weird, but fixed and absolute make things messy and this is only for edge cases.

Big notifications now look like this:
image
And the close button doesn't scroll out of view:
image

If we use position: fixed or absolute then the button is removed from the dom flow and things go underneath it. This is a much easier fix for this edge case.

And keep the close button in view. Using sticky is a bit weird, but fixed and absolute make things messy and this is only for edge cases.
@github-actions github-actions bot added the 📦 Lexbox issues related to any server side code, fw-headless included label Oct 6, 2025
@github-actions
Copy link

github-actions bot commented Oct 6, 2025

UI unit Tests

 1 files  ±  0   3 suites   - 42   0s ⏱️ -28s
10 tests  - 101  10 ✅  - 101  0 💤 ±0  0 ❌ ±0 
10 runs   - 150  10 ✅  - 150  0 💤 ±0  0 ❌ ±0 

Results for commit e2d0e3a. ± Comparison against base commit ef484ee.

This pull request removes 111 and adds 10 tests. Note that renamed tests count towards both.
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > accepts parent values when not dirty
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > does NOT guard against stomping deep changes, because StompGuard can't detect them
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > ignores parent values when dirty
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > initializes with the parent value
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > keeps subscribers up to date when it becomes dirty
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > pushs changes to parent
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > reverts new parent values when ignored
src/lib/components/stomp/stomp-guard.svelte.test.ts ‑ StompGuard > starts accepting parent changes again after a flush
src/lib/components/ui/format/format-duration.test.ts ‑ formatDuration > formats durations
src/lib/components/ui/format/format-duration.test.ts ‑ formatDuration > formats durations with smallest unit
…
src/index.test.ts ‑ password hashing > can hash a pw using sha1
src/lib/i18n/i18n.test.ts ‑ buildRegionalLocaleRegex > should find all regional locales for available locales
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return en by default
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return en if no user locale is provided and acceptLanguageHeader does not have any supported locales
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return regional locale from acceptLanguageHeader if it has a higher quality rating than the regionless locale
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return supported locale from acceptLanguageHeader with highest quality rating if no user locale is provided
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return user locale if acceptLanguageHeader does not provide a regional/more specific locale
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return user locale if acceptLanguageHeader does not provide a regional/more specific locale with a higher quality rating
src/lib/i18n/i18n.test.ts ‑ pickBestLocale > should return user locale if acceptLanguageHeader is not provided
src/lib/user.test.ts ‑ jwtToUser > should convert a jwt token to a LexAuthUser

@github-actions
Copy link

github-actions bot commented Oct 6, 2025

C# Unit Tests

130 tests  ±0   130 ✅ ±0   19s ⏱️ ±0s
 20 suites ±0     0 💤 ±0 
  1 files   ±0     0 ❌ ±0 

Results for commit e2d0e3a. ± Comparison against base commit ef484ee.

@coderabbitai
Copy link

coderabbitai bot commented Oct 6, 2025

📝 Walkthrough

Walkthrough

Updates the Notify.svelte component’s markup and classes to constrain toast container height, enable vertical scrolling within notifications, and keep the close button visible via sticky positioning. No logic or data flow changes.

Changes

Cohort / File(s) Change summary
Notify UI layout adjustments
frontend/src/lib/notify/Notify.svelte
Added max height constraint to toast container, enabled per-notification vertical scrolling (overflow-y-auto), and made the close button sticky to remain visible while scrolling. No control flow or data handling changes.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

A nibble of UI, a hop of delight,
My toasts now scroll through the saffron night.
A sticky close, no more chase-and-click—
I twitch my nose; it’s tidy and slick.
In a warren of alerts, neat and tall,
Every little message finds room for all. 🐇✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title Check ✅ Passed The title clearly summarizes the primary change by stating that Lexbox notifications will become scrollable when they exceed the available height, matching the implementation of overflow behavior in the PR without extraneous detail.
Description Check ✅ Passed The description directly explains the rationale and implementation details for making notifications scrollable and keeping the close button visible, and includes relevant examples, so it is clearly related to the changeset.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-lexbox-notifications-overflow-page

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ef484ee and e2d0e3a.

📒 Files selected for processing (1)
  • frontend/src/lib/notify/Notify.svelte (1 hunks)
🔇 Additional comments (3)
frontend/src/lib/notify/Notify.svelte (3)

11-11: LGTM!

The max-h-full constraint correctly prevents the toast container from exceeding viewport height, which is essential for the scrolling behavior to work properly.


18-18: LGTM!

Adding overflow-y-auto enables vertical scrolling for tall notifications, which aligns with the PR objective.


20-20: Add aria-label for accessibility and verify sticky positioning behavior.

The close button has two concerns:

  1. Accessibility: The button contains only the "✕" character with no accessible label, preventing screen reader users from understanding its purpose.

  2. CSS: Using both top-0 and bottom-0 with sticky positioning is unusual. Browsers typically prioritize top when both are specified, potentially making bottom-0 redundant. While this may work as intended based on your testing, the behavior might vary across browsers or viewport sizes.

Apply this diff to add an accessible label:

-<button onclick={() => removeNotification(note)} class="btn btn-circle btn-sm btn-ghost sticky top-0 bottom-0">✕</button>
+<button onclick={() => removeNotification(note)} class="btn btn-circle btn-sm btn-ghost sticky top-0 bottom-0" aria-label="Close notification">✕</button>

Please verify that the sticky top-0 bottom-0 combination works as expected across different browsers and notification heights. If bottom-0 is not needed, consider removing it for clarity:

-<button onclick={() => removeNotification(note)} class="btn btn-circle btn-sm btn-ghost sticky top-0 bottom-0">✕</button>
+<button onclick={() => removeNotification(note)} class="btn btn-circle btn-sm btn-ghost sticky top-0" aria-label="Close notification">✕</button>

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@myieye myieye merged commit 497cc74 into develop Oct 8, 2025
14 checks passed
@myieye myieye deleted the fix-lexbox-notifications-overflow-page branch October 8, 2025 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

📦 Lexbox issues related to any server side code, fw-headless included

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants