-
Notifications
You must be signed in to change notification settings - Fork 86
Update Flash to tabbed and add settings option. #2509
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Update Flash to tabbed and add settings option. #2509
Conversation
WalkthroughThree new boot settings configuration pages are added to the dynamix plugin: Boot.page establishes a settings menu entry, Bootredirect.page implements browser redirection logic, and Flash.page is modified to support tabbed navigation. These changes facilitate boot settings management within the plugin infrastructure. Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
|
Need to look at icon option. |
🔧 PR Test Plugin AvailableA test plugin has been generated for this PR that includes the modified files. Version: 📥 Installation Instructions:Install via Unraid Web UI:
Alternative: Direct Download
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/Boot.page (1)
1-4: Define the content or child pages for this xmenu.This file creates a "Boot Settings" entry in the OtherSettings menu with
Type="xmenu", but contains no actual content or child page definitions.For an xmenu to be functional, it typically needs:
- Child pages with
Menu="Boot"(to appear as sub-items), or- Tabs (if using Tabs="true"), or
- Some content to display
Questions for clarification:
- Is this meant to be a container for Bootredirect.page (which has
Menu="Boot")?- Should this have
Tabs="true"like Flash.page?- What settings/pages are intended to appear under "Boot Settings"?
Since this PR is in draft status, please clarify the intended structure so I can provide more specific guidance on the implementation.
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
emhttp/plugins/dynamix/Boot.pageemhttp/plugins/dynamix/Bootredirect.pageemhttp/plugins/dynamix/Flash.page
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-03-27T22:04:34.550Z
Learnt from: zackspear
Repo: unraid/webgui PR: 2099
File: emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php:13-19
Timestamp: 2025-03-27T22:04:34.550Z
Learning: The file emhttp/plugins/dynamix.my.servers/include/web-components-extractor.php is synced from another repository and should not be modified directly in the webgui repository.
Applied to files:
emhttp/plugins/dynamix/Bootredirect.page
🔇 Additional comments (1)
emhttp/plugins/dynamix/Flash.page (1)
2-2: No action needed. TheTabs="true"attribute is properly configured. Three tab pages withMenu="Flash"are already defined in the codebase: Syslinux.page, Grub.page, and FlashInfo.page. In this framework, tab pages are defined as separate .page files with the Menu attribute, not within the parent xmenu file itself.Likely an incorrect or invalid review comment.
| Menu="Boot" | ||
| --- | ||
| <script>window.location.href = '/Flash?name=flash';</script> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reconsider the redirect approach for better UX.
This file creates a "Boot" menu entry that immediately redirects users to /Flash?name=flash. This approach has several concerns:
- User confusion: Clicking a "Boot" menu item and being instantly redirected to "Flash" may be disorienting
- Unnecessary indirection: If the goal is to show the Flash page, why not configure the menu to link directly to it?
- Architecture: If Boot.page is meant to be a container for Boot Settings (as seen in the other file), having a separate redirect page seems redundant
Consider instead:
- Configuring the Boot menu entry to link directly to
/Flash?name=flashwithout a redirect page - Or making Boot.page the primary page and having Flash as a tab under it
- Or clarifying the intended navigation structure between Boot.page and this redirect
Could you clarify the intended menu structure? I can help suggest a cleaner implementation once the goal is clear.
|
One day we're going to have to revisit tab vs non tabbed altogether. We're inconsistent since some pages force it and others don't. For myself, I'm in the camp of remove non-tabbed altogether in the UI pages. But this came up a couple months ago in a different context. |
Summary by CodeRabbit
New Features
UI Improvements
✏️ Tip: You can customize this high-level summary in your review settings.