-
Notifications
You must be signed in to change notification settings - Fork 119
core: frontend: BrIFrame: Change hiding approach #3714
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
Merged
joaomariolago
merged 1 commit into
bluerobotics:master
from
patrickelectric:fix-terminal
Jan 12, 2026
Merged
core: frontend: BrIFrame: Change hiding approach #3714
joaomariolago
merged 1 commit into
bluerobotics:master
from
patrickelectric:fix-terminal
Jan 12, 2026
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Reviewer's guide (collapsed on small PRs)Reviewer's GuideBrIframe now keeps the iframe always present and uses absolute positioning with z-index to layer the loading indicators above it instead of conditionally hiding the iframe with v-show. State diagram for BrIframe iframe loading lifecyclestateDiagram-v2
[*] --> Initializing
Initializing: iframe_loaded = false
Initializing: iframe rendered behind overlay
Initializing --> LoadingOverlayVisible: gl_compatible = false
Initializing --> WebGLOvelayVisible: gl_compatible = true
LoadingOverlayVisible: spinning_logo visible at z-index 2
WebGLOvelayVisible: webglCanvas visible at z-index 2
LoadingOverlayVisible --> IframeReady: iframe loadFinished()
WebGLOvelayVisible --> IframeReady: iframe loadFinished()
IframeReady: iframe_loaded = true
IframeReady: iframe remains at z-index 1
IframeReady: overlays not rendered
IframeReady --> [*]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey - I've found 1 issue, and left some high level feedback:
- Consider moving the new inline positioning/z-index styles into CSS classes to keep the template more readable and avoid scattering layout logic across inline style attributes.
- With the iframe now always visible but layered underneath, ensure that the overlay elements (spinner/WebGL canvas) are removed or made non-interactive (
pointer-events: none) as soon as possible so they don't inadvertently block user interaction or accessibility tooling that expects to reach the iframe. - The absolute positioning on the
canvasandspinning-logoelements sets top/left but not width/height for the canvas, which may lead to inconsistent sizing compared to the spinner and iframe; consider explicitly sizing the canvas to fill the container as you do with the spinner.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider moving the new inline positioning/z-index styles into CSS classes to keep the template more readable and avoid scattering layout logic across inline style attributes.
- With the iframe now always visible but layered underneath, ensure that the overlay elements (spinner/WebGL canvas) are removed or made non-interactive (`pointer-events: none`) as soon as possible so they don't inadvertently block user interaction or accessibility tooling that expects to reach the iframe.
- The absolute positioning on the `canvas` and `spinning-logo` elements sets top/left but not width/height for the canvas, which may lead to inconsistent sizing compared to the spinner and iframe; consider explicitly sizing the canvas to fill the container as you do with the spinner.
## Individual Comments
### Comment 1
<location> `core/frontend/src/components/utils/BrIframe.vue:13-17` </location>
<code_context>
subtitle="Loading external application..."
+ style="position: absolute; top: 0; left: 0; width: 100%; height: 100%; z-index: 2;"
/>
<canvas
v-if="gl_compatible && !iframe_loaded"
ref="webglCanvas"
+ style="position: absolute; top: 0; left: 0; z-index: 2;"
/>
<iframe
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Canvas overlay might not cover the full iframe area as intended.
The iframe sets `width: 100%; height: 100%`, but the canvas overlay only sets position and z-index. To ensure the overlay matches the iframe area in all layouts, add `width: 100%; height: 100%` to the canvas style or an appropriate class.
```suggestion
<canvas
v-if="gl_compatible && !iframe_loaded"
ref="webglCanvas"
style="position: absolute; top: 0; left: 0; width: 100%; height: 100%; z-index: 2;"
/>
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Instead of hiding the iframe with v-show (which sets display: none and gives zero dimensions), the iframe is now always visible but layered behind the loading animation using z-index positioning. Signed-off-by: Patrick José Pereira <patrickelectric@gmail.com>
3a7c51b to
97452ab
Compare
joaomariolago
approved these changes
Jan 12, 2026
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Instead of hiding the iframe with v-show (which sets display: none and gives zero dimensions), the iframe is now always visible but layered behind the loading animation using z-index positioning.
Summary by Sourcery
Enhancements: