Prototype for using HTML's dialog element instead of jBox for modal dialogs (OSD tab) #4484
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the 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. |
|
|
Preview URL: https://c58f3441.betaflight-configurator.pages.dev |
@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!