Replace all 4 instances of jBox-style modal dialog boxes with HTML dialog element #4484
Conversation
WalkthroughThis update migrates modal dialogs from the third-party "jBox" library to native HTML Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI (Button/Trigger)
participant initializeModalDialog (Utility)
participant HTMLDialogElement
User->>UI (Button/Trigger): Clicks to open modal
UI (Button/Trigger)->>initializeModalDialog: Calls with selectors, title key, etc.
initializeModalDialog->>HTMLDialogElement: Prepares dialog, sets title, binds close event
initializeModalDialog-->>UI (Button/Trigger): Returns dialog element
UI (Button/Trigger)->>HTMLDialogElement: Calls showModal()
User->>HTMLDialogElement: Interacts and closes dialog
HTMLDialogElement->>initializeModalDialog: Triggers close callback (if provided)
This diagram illustrates the new standardized dialog flow using the utility function and native HTML dialogs. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
@DavidAnson thank you. Some comments:
Before: After: |
|
Thank you for the feedback! (Your first two sections are blank, but I assume that’s because you had no comment on those points in my list.) The scrollbar is present in my implementation because it is present in the current implementation. The height of the dialog is fixed and so the few elements at the bottom are hidden by default. If I remove the height restriction, I think it will automatically grow as necessary. But I will leave the overflow property so people with short screens/windows will still be able to get to everything. As I recall, there are two or three other modal dialogs in the app. Would you rather see this PR go in just changing this one and then follow up with the others OR would you rather have a PR that touches all of them at once? I could argue in favor of either approach. :) |
|
First two points are clear. Removing scrolling is a suggestion for improvement and perhaps should be done separate. |
|
@DavidAnson any updates :) |
|
No, sorry, been unable to work on this for the past few days. I should have time this week. |
|
Is this not ready now ? |
|
@haslinghuis I've migrated all four dialogs, but it needs a little bit more work. I've been busy, but might do some more tonight. In particular, I'd like to pull some of the styling into a common theme and the OSD dialog has a couple of elements in the wrong place right now. Interestingly, there appears to be a green-screen effect for the custom OSD image which is visible in my branch, but does not show up for me in the public version. I solved one or two similar things by updating CSS selectors and assume something similar needs to be done here. I figured I would try to restore the green effect if I could figure out what it was meant to look like. Here are my complete notes: |
DavidAnson
changed the title
|
@haslinghuis, this is ready for review! I’ve included the commit notes below. Reviewers, please be aware that all testing has been done with the Virtual device - test this with real hardware/input including font files and custom images! Replace all 4 instances of jBox-style modal dialog boxes with HTML dialog element Notes:
|
|
^^ Regarding the green color for custom images, I saw it in a Joshua Bardwell video from a few years ago which was a nice reassurance that it did used to work at some point in time. :) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
locales/en/messages.json (1)
5311-5314: Provide clearer context for translators and verify usage of the new keyThe
descriptionfield is quite generic; specifying "for the OSD Font Manager modal dialog" will give translators better context. Also, confirm that this key is actually referenced in the code to avoid dead entries.Proposed diff:
"osdSetupFontManagerTitle": { "message": "OSD Font Manager", - "description": "Dialog title" + "description": "Dialog title for the OSD Font Manager modal dialog" },Verify usage with:
#!/bin/bash # Search for occurrences of the new localization key rg "osdSetupFontManagerTitle" -n .src/css/tabs/cli.less (1)
78-89: Scope the preview textarea selector to prevent global leakage.The global
textarea#previewrule now applies everywhere rather than only within the CLI dialog, risking unintended side effects in other parts of the app. It’s safer to scope it under the CLI context (or the dialog wrapper), for example:- textarea#preview { + .tab-cli textarea#preview { background-color: rgba(0, 0, 0, 0.75); width: 100%; resize: none; overflow-y: scroll; overflow-x: hidden; font-family: monospace; color: white; box-sizing: border-box; padding: 5px; margin-bottom: 5px; }src/css/tabs/osd.less (1)
5-13: Replace negative margin hack with flex-centering for.progressLabel.Using
margin-top: -22pxis brittle and may break if content height or font-size changes. Instead, consider using a flex container or centering with relative positioning for more robust vertical alignment:.tab-osd .info { - position: relative; + display: flex; + align-items: center; width: 100%; .progressLabel { - position: absolute; - width: 100%; - height: 26px; - margin-top: -22px; + width: 100%; + line-height: 26px; /* center text vertically */ text-align: center; color: white; font-weight: bold; a { ... } } }src/js/utils/initializeModalDialog.js (1)
54-54: Consider using optional chaining for cleaner code.The logical AND pattern can be simplified using optional chaining for better readability.
Apply this diff to use optional chaining:
- onClose && onClose(); + onClose?.();
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
locales/en/messages.json(1 hunks)src/css/main.less(1 hunks)src/css/tabs/cli.less(1 hunks)src/css/tabs/osd.less(3 hunks)src/js/main.js(0 hunks)src/js/tabs/cli.js(2 hunks)src/js/tabs/osd.js(2 hunks)src/js/tabs/power.js(2 hunks)src/js/utils/initializeModalDialog.js(1 hunks)src/tabs/cli.html(1 hunks)src/tabs/osd.html(3 hunks)src/tabs/power.html(3 hunks)test/setup.js(0 hunks)
💤 Files with no reviewable changes (2)
- src/js/main.js
- test/setup.js
🧰 Additional context used
🪛 HTMLHint (1.5.0)
src/tabs/osd.html
[error] 155-155: Duplicate of attribute name [ class ] was found.
(attr-no-duplication)
🪛 Biome (1.9.4)
src/js/utils/initializeModalDialog.js
[error] 54-54: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (15)
src/css/tabs/osd.less (1)
365-368: Verify uniqueness of the#font-logoID and consider switching to a class.IDs must be unique across the document. If the font-logo block can appear in multiple contexts, convert
#font-logoto a class (e.g.,.font-logo) to avoid collision.src/css/main.less (1)
44-50: New.html-dialogclasses correctly standardize dialog styling.The additions for
.html-dialogand.html-dialog-contentproperly override padding and border colour without interfering with the default dialog styles. Looks good.src/js/tabs/osd.js (2)
19-19: LGTM! Import statement follows established pattern.The import of the
initializeModalDialogutility function is correctly implemented and aligns with the PR's objective to standardize modal dialog handling across the application.
2764-2765: Excellent migration to native HTML dialog elements.The replacement of jBox modal instantiation with the
initializeModalDialogutility function is well-implemented. The parameters are appropriate and this change successfully achieves the PR's objective of migrating to native HTML dialog elements while maintaining functionality and improving accessibility.src/tabs/cli.html (1)
16-27: LGTM! Clean migration to native dialog element.The restructuring correctly converts the snippet preview from a hidden div to a semantic HTML dialog element while preserving all functionality and content. The addition of
closedby="any"and appropriate CSS classes aligns with the new modal system.src/tabs/osd.html (1)
154-202: Successful dialog migration with improved semantic structure.The font manager dialog has been properly converted to use native HTML dialog elements. The content organization and styling maintain the existing functionality while improving accessibility through semantic markup.
src/js/tabs/cli.js (3)
15-15: Correct import for the new modal system.The import of
initializeModalDialogutility properly replaces the jBox dependency for modal dialog handling.
154-163: Proper migration from jBox to native dialog initialization.The refactoring correctly uses the new
initializeModalDialogutility and properly handles the event registration outside of the dialog creation callback. The method change fromopen()toshowModal()aligns with native dialog API.
528-546: Appropriate removal of jBox-specific cleanup code.The cleanup method correctly removes jBox modal destruction logic since native HTML dialogs are automatically managed by the browser and don't require explicit cleanup.
src/tabs/power.html (1)
200-263: Consistent and correct dialog element migration.Both calibration manager dialogs have been properly converted to semantic HTML dialog elements with appropriate attributes and CSS classes. The structure maintains existing functionality while improving accessibility and standardizing modal behavior.
src/js/utils/initializeModalDialog.js (1)
41-64: Well-structured utility function for dialog initialization.The function properly handles all aspects of modal dialog setup including title bar creation, event handling, and activation button binding. The scroll reset feature is a nice touch for user experience.
src/js/tabs/power.js (4)
9-9: LGTM: Clean import replacementThe import change from jBox to the new initializeModalDialog utility is consistent with the migration goals.
340-346: Verify the callback behavior for calibrationManagerThe modal initialization looks correct, but please confirm the callback logic:
() => calibrationconfirmed || TABS.power.initialize()will only callTABS.power.initialize()whencalibrationconfirmedis false (using short-circuit evaluation). Is this the intended behavior for the close callback?
348-354: LGTM: Confirmation dialog migrationThe calibrationManagerConfirmation dialog initialization is clean and the callback correctly closes the parent dialog.
543-545: LGTM: Simplified cleanup aligns with native dialogsRemoving explicit modal destruction from cleanup is appropriate since native HTML dialog elements don't require manual cleanup like jBox modals did.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/js/utils/initializeModalDialog.js (2)
54-54: Use optional chaining for cleaner code.The static analysis tool correctly suggests using optional chaining instead of the logical AND operator.
Apply this diff to use optional chaining:
- onClose && onClose(); + onClose?.();
13-25: Consider extracting inline styles to CSS classes.The extensive use of inline styles makes the code harder to maintain and doesn't follow separation of concerns best practices.
Consider moving these styles to CSS classes in the main stylesheet and referencing them here. This would improve maintainability and allow for easier theming adjustments.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/utils/initializeModalDialog.js(1 hunks)src/tabs/osd.html(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/tabs/osd.html
🧰 Additional context used
🪛 Biome (1.9.4)
src/js/utils/initializeModalDialog.js
[error] 54-54: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (1)
src/js/utils/initializeModalDialog.js (1)
41-64: Overall implementation looks solid.The main function logic is well-structured and handles the core requirements for modal dialog initialization. The approach of using native HTML dialog elements is modern and accessible.
|
Please rebase on master to include latest changes to facilitate testing. |
I'll do that tonight, but FYI I did so before converting from a draft PR and it looks like there are only four newer commits to the branch. |
|
@DavidAnson yes asking as it changed MSP parameters |
…alog element Notes: - The Power & Battery (2), OSD, and CLI tabs have been updated accordingly - HTML default control behavior was used whereever possible - HTML indent for the dialog tag is deliberately off to minimize diff noise in GitHub - Layout is not 100% identical, but spiratually the same - in most cases no changes were needed - OSD’s Font Manager dialog didn’t transition cleanly, so includes some slight restyling - OSD Font Manager previously had a green background/color for the graphic - that “just works” now - Specific cleanup (i.e., calling destroy()) does not seem necessary and was removed - Removal of jBox from setup.js and global scope seems not to affect jBox tooltips - Replacement of single quotes with double in main.js was done automatically by Prettier
|
@haslinghuis, rebased and updated! |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/js/utils/initializeModalDialog.js (1)
60-60: Add null safety for dialogContainerElement.Based on previous discussion, while jQuery handles null selectors gracefully,
dialogContainerElementcould beundefinedifdialog.children().first().get(0)returns no element, which would causescroll(0, 0)to throw an error.- // Reset any previous scrolling - dialogContainerElement.scroll(0, 0); + // Reset any previous scrolling + dialogContainerElement?.scroll(0, 0);
🧹 Nitpick comments (1)
src/js/utils/initializeModalDialog.js (1)
54-54: Use optional chaining for cleaner code.The static analysis tool correctly identified that this can be simplified using optional chaining.
- dialogElement.addEventListener("close", () => { - onClose && onClose(); - }); + dialogElement.addEventListener("close", () => { + onClose?.(); + });
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
locales/en/messages.json(1 hunks)src/css/main.less(1 hunks)src/css/tabs/cli.less(1 hunks)src/css/tabs/osd.less(3 hunks)src/js/main.js(0 hunks)src/js/tabs/cli.js(2 hunks)src/js/tabs/osd.js(2 hunks)src/js/tabs/power.js(2 hunks)src/js/utils/initializeModalDialog.js(1 hunks)src/tabs/cli.html(1 hunks)src/tabs/osd.html(3 hunks)src/tabs/power.html(3 hunks)test/setup.js(0 hunks)
💤 Files with no reviewable changes (2)
- src/js/main.js
- test/setup.js
🚧 Files skipped from review as they are similar to previous changes (10)
- src/css/tabs/cli.less
- src/js/tabs/power.js
- src/tabs/power.html
- src/tabs/cli.html
- src/css/main.less
- src/js/tabs/cli.js
- src/js/tabs/osd.js
- src/tabs/osd.html
- src/css/tabs/osd.less
- locales/en/messages.json
🧰 Additional context used
🧠 Learnings (1)
src/js/utils/initializeModalDialog.js (2)
Learnt from: DavidAnson
PR: betaflight/betaflight-configurator#4484
File: src/js/utils/initializeModalDialog.js:18-18
Timestamp: 2025-06-21T06:29:10.886Z
Learning: In the initializeModalDialog utility function, the `find("#dialogclose")` call is scoped to the specific `dialogTitleBar` jQuery object being created, not the entire DOM. This means multiple modal dialogs can safely use the same "dialogclose" ID because each find operation only searches within its own title bar instance.
Learnt from: DavidAnson
PR: betaflight/betaflight-configurator#4484
File: src/js/utils/initializeModalDialog.js:57-61
Timestamp: 2025-06-21T06:31:57.094Z
Learning: jQuery handles null and empty selectors gracefully by returning an empty jQuery object. Calling methods like `.on()` on empty jQuery objects is a no-op and doesn't throw errors, so null checks for jQuery selectors are often unnecessary.
🪛 Biome (1.9.4)
src/js/utils/initializeModalDialog.js
[error] 54-54: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (2)
src/js/utils/initializeModalDialog.js (2)
11-30: LGTM! Well-structured dialog title bar creation.The function creates a clean, accessible title bar with proper localization and event handling. The scoped ID usage is safe as discussed in previous reviews.
41-64: Excellent migration from jBox to native dialogs.This utility function provides a clean abstraction for initializing native HTML dialog elements, maintaining the functionality while leveraging browser-native behavior. The approach aligns well with the PR objectives of simplifying modal invocation and improving accessibility.
|
this button does not respond ?
|
|
Possibly if is hooked up via query and the selector changed/got broken. Will have a look tonight! |
…rlapping layout; replace with CSS grid.
@haslinghuis, this should be fixed now, sorry. It was an issue with overlapping content trying to center the progress text over the progress bar. I converted to CSS grid which handles the scenario better. |
|
can this be taller to avoid scrollbars? (or is this outside of scope?) |
|
@nerdCopter I knew you were going to ask that: #4484 (comment) 😛 |
src/tabs/osd.html
Outdated
| @@ -151,7 +151,8 @@ <h1 class="tab_title"> | |||
| </div> | |||
|
|
|||
| <!-- Font Manager dialog --> | |||
| <div id="fontmanagercontent" style="display:none; width:720px;"> | |||
| <dialog closedby="any" id="fontmanagerdialog" class="html-dialog" style="width: 750px;"> | |||
| <div class="html-dialog-content" style="height: 430px; overflow-y: scroll;"> | |||
There was a problem hiding this comment.
Perhaps
| <div class="html-dialog-content" style="height: 430px; overflow-y: scroll;"> | |
| <div class="html-dialog-content" style="height: 530px; overflow-y: scroll;"> |
There was a problem hiding this comment.
Without scroll
| <div class="html-dialog-content" style="height: 430px; overflow-y: scroll;"> | |
| <div class="html-dialog-content" style="height: 530px"> |
|
Haha, okay, will do. :) I think removing the height entirely will allow the dialog to size itself as in the other cases. |
|
@haslinghuis / @nerdCopter No more default scroll bar for OSD Font Manager dialog. :) |
|
interestingly i still see unusable scroll (Preview URL) |
|
@nerdCopter, I left the overflow attribute in case someone had a short screen and needed to scroll. Some browsers hide the scroll bar in that case. I can remove the overflow attribute entirely. |
… to avoid even a disabled scroll bar on some platforms/browsers (PR feedback).
|
Preview URL: https://4cedb416.betaflight-configurator.pages.dev |
|
|
@nerdCopter, that disabled scrollbar is browser/platform-dependent, I think. It was not present on Chrome/Chromebook, but it should be gone now based on the CSS change I just made. |
@haslinghuis, you asked me to look at removing jBox last week. Here's a first draft of replacing the OSD tab's jBox modal dialog with the HTML
dialogelement. Please have a look and give it a try and let me know what you think of this basic approach.Notes:
dialogelement, so there's very little change there or to convert.modal.jsfile, probably. The net result is that it's a little bit simpler to invoke this than the jBox modal. (Assuming there aren't weird gotchas with the other couple of modals in the project.)Escor clicking the close button works. As a browser-native element, tab stops and accessibility should work correctly by default.flexboxinstead offloat, but I was lazy in the prototype. :) The hit box for the close button would be easier/larger with that approach and probably not need the negative margin trick I used.I think those are the high points. Please let me know your thoughts!
Summary by CodeRabbit
New Features
Refactor
Style
Chores