Skip to content

Conversation

@Ajit-Mehrotra
Copy link
Contributor

@Ajit-Mehrotra Ajit-Mehrotra commented Dec 9, 2025

Purpose

Unraid API is refactoring the notification pane to use NuxtUI in an upcoming release (not ready yet).

NuxtUI allows notification toasts to be positioned in the bottom-center and top-centerpositions.

In order to give Unraid users the most flexibility, we're requesting these changes are applied to the webgui.
image

Please Note

These changes are not ready to be merged yet. Please coordinate with the API team. @zackspear is aware of both the API and Webgui changes.

Summary by CodeRabbit

  • New Features
    • Updated notification display position options: replaced the single center option with bottom-center and top-center choices for improved positioning flexibility.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 9, 2025

Walkthrough

The Notifications configuration page has been updated to replace a single "center" display position option with two distinct alternatives: "bottom-center" and "top-center", providing users with more granular control over notification placement.

Changes

Cohort / File(s) Change Summary
Notification display position options
emhttp/plugins/dynamix/Notifications.page
Replaced single center option with bottom-center and top-center position choices for notifications

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Poem

🐰 A notification finds its place,
No longer just the center's space!
Now top and bottom have their say,
Flexibility greets the day!
Hop forward, changes light and bright! ✨

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title accurately describes the main change—adding new notification positions (bottom-center and top-center) in preparation for an upcoming Unraid API refactor.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
emhttp/plugins/dynamix/styles/default-base.css (1)

142-150: Form control scoping away from .unapi looks correct; sanity-check real .unapi views

The :where(:not(.unapi *)) pattern on inputs, textareas, buttons, selects, and their focus/hover/disabled states cleanly prevents these legacy base styles from leaking into .unapi descendants while preserving the original selector specificity. This is a solid way to shield NuxtUI (and other .unapi content) from global webgui styling.

Two follow-ups worth verifying:

  • In actual .unapi pages, confirm that controls truly rely only on their own NuxtUI styles and aren’t still being hit indirectly by other, more specific legacy rules.
  • If .unapi content is ever rendered inside .Theme--sidebar or other nested contexts that still use bare input[type="..."]/select selectors, consider mirroring this :where(:not(.unapi *)) trick there too to avoid future bleed-over.

Also applies to: 184-189, 253-260, 263-334, 338-377, 419-424

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d0e94e4 and a417db7.

📒 Files selected for processing (3)
  • .vscode/settings.json (1 hunks)
  • emhttp/plugins/dynamix/Notifications.page (1 hunks)
  • emhttp/plugins/dynamix/styles/default-base.css (8 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-02T21:54:23.875Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-622
Timestamp: 2025-06-02T21:54:23.875Z
Learning: CSS nesting is a modern CSS feature that is well-supported in browsers and allows nested selectors like input[type="button"] inside parent blocks like .sa-button-container. This is valid CSS syntax and should not be flagged as an error.

Applied to files:

  • emhttp/plugins/dynamix/styles/default-base.css
📚 Learning: 2025-06-02T21:51:01.477Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2222
File: emhttp/plugins/dynamix/styles/jquery.sweetalert.css:610-640
Timestamp: 2025-06-02T21:51:01.477Z
Learning: In the Unraid webgui codebase, SweetAlert button styling is handled through CSS inheritance from general button selectors in default-base.css rather than component-specific `.sweet-alert button` selectors. SweetAlert buttons inherit styling from the general `button`, `button[type="button"]`, `input[type="button"]`, and `a.button` selectors.

Applied to files:

  • emhttp/plugins/dynamix/styles/default-base.css
📚 Learning: 2025-09-05T19:26:36.587Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2354
File: emhttp/plugins/dynamix/ShareEdit.page:0-0
Timestamp: 2025-09-05T19:26:36.587Z
Learning: In emhttp/plugins/dynamix/ShareEdit.page, the clone-settings div was moved outside the form element and both are wrapped in a div.relative container to prevent event bubbling issues while maintaining proper positioning.

Applied to files:

  • emhttp/plugins/dynamix/Notifications.page
🔇 Additional comments (3)
.vscode/settings.json (1)

5-5: Disabling Intelephense formatting is fine; just align on formatter expectations

Turning off intelephense.format.enable at the workspace level makes sense given its issues with .page files; just ensure the team is standardizing on Prettier/php-cs-fixer (or similar) so PHP (and .page) formatting stays consistent across devs.

emhttp/plugins/dynamix/Notifications.page (1)

115-123: New bottom-center/top-center options look good; verify value mapping and legacy configs

Adding bottom-center and top-center here matches the NuxtUI-style positions and keeps the UI straightforward. Two things to double-check with the API side before this ships out of draft:

  • Confirm the backend / new notification system uses the exact same string values ("bottom-center" and "top-center") so there’s no mismatch between saved config and runtime behavior.
  • Decide how existing "center" (legacy) values in dynamix.cfg should migrate (e.g., map to bottom-center by default) so current users don’t silently flip to an unintended corner when they next hit Apply.
emhttp/plugins/dynamix/styles/default-base.css (1)

956-973: Base table styles now excluded from .unapi; confirm Nuxt-side tables are covered

Scoping the generic table / thead / tbody rules with :where(:not(.unapi *)) is consistent with the form control approach and should stop legacy table chrome from bleeding into .unapi content, while still letting table.unraid, table.dashboard, etc., style the existing pages.

Just make sure:

  • Any tables rendered under .unapi are either fully controlled by Nuxt-side styles or have their own explicit CSS, since they’ll no longer inherit these base table rules.
  • There isn’t a hybrid case where a legacy table.unraid might intentionally be nested into .unapi, as that would still be partially styled by the more specific .unraid rules.

@zackspear zackspear self-requested a review December 9, 2025 21:11
@Ajit-Mehrotra Ajit-Mehrotra force-pushed the feat/notification-positions branch from a417db7 to 8b44c8a Compare December 9, 2025 21:32
@zackspear
Copy link
Contributor

This requires Nuxt UI dependency to be upgraded to the latest version. We'll hold on this until that updated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants