Skip to content

Popups and Tooltips in full screen mode #8933

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

Open
wants to merge 7 commits into
base: develop
Choose a base branch
from

Conversation

SasLord
Copy link
Member

@SasLord SasLord commented May 14, 2025

Popups:
screen-fs-popups

Tooltips:
screen-fs-tooltips

Copy link

Connected to Huly®: UBERF-10588

Copy link
Member

@haiodo haiodo left a comment

Choose a reason for hiding this comment

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

I do not understand why we need this hacks.

@SasLord SasLord changed the title Pop ups in full screen mode Popups and Tooltips in full screen mode May 15, 2025
@SasLord SasLord force-pushed the popups-in-fullscreen branch from 5f20200 to 7d145b3 Compare May 15, 2025 15:57
@SasLord SasLord requested a review from haiodo May 15, 2025 15:57
@SasLord SasLord force-pushed the popups-in-fullscreen branch from 7d145b3 to ded4950 Compare May 15, 2025 16:08
@aonnikov aonnikov requested a review from Copilot May 16, 2025 07:05
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request focuses on improving the handling of popups and tooltips in full screen mode. Key changes include updating the return type for room settings, adding full screen checks in tooltip and popup components, and simplifying popup rendering in the control bar.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
plugins/love-resources/src/utils.ts Changed the return type of showRoomSettings to Promise
plugins/love-resources/src/components/RoomLanguageSelector.svelte Added an import for isFullScreen
plugins/love-resources/src/components/ControlBar.svelte Removed full screen branch logic and streamlined popup rendering
packages/ui/src/components/TooltipInstance.svelte Introduced a fullScreen prop and added conditional logic based on full screen state
packages/ui/src/components/Popup.svelte Added a fullScreen prop and updated popup filtering with a checkFS function
Comments suppressed due to low confidence (1)

packages/ui/src/components/TooltipInstance.svelte:69

  • [nitpick] Consider renaming 'checkFS' to a more descriptive name like 'shouldDisplayTooltip' to better convey its purpose.
const checkFS = (): boolean => (fullScreen && !document.fullscreen) || (!fullScreen && document.fullscreen)

@SasLord SasLord force-pushed the popups-in-fullscreen branch from acf2555 to f058999 Compare May 16, 2025 09:00
@SasLord SasLord requested a review from Copilot May 16, 2025 09:01
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds fullscreen-aware handling for popups and tooltips across the app.

  • Introduces an isFullScreen flag and updates components to conditionally render popups/tooltips based on actual fullscreen state.
  • Refactors ControlBar.svelte to remove custom popup logic and use the shared <Popup fullScreen/> and <TooltipInstance fullScreen/> components.
  • Enhances TooltipInstance.svelte and Popup.svelte with a new fullScreen prop and filtering logic to show/hide instances appropriately.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
plugins/love-resources/src/components/RoomLanguageSelector.svelte Added isFullScreen import (not yet used)
plugins/love-resources/src/components/ControlBar.svelte Removed bespoke popup logic; always render shared fullscreen popup/tooltip components
packages/ui/src/components/TooltipInstance.svelte Added fullScreen prop and checkFS logic; wrapped rendering in conditional based on fullscreen state
packages/ui/src/components/Popup.svelte Added fullScreen prop and checkFS filtering when listing popup instances
Comments suppressed due to low confidence (5)

packages/ui/src/components/TooltipInstance.svelte:254

  • [nitpick] The new fullscreen checks in hideTooltip and the rendering conditions aren’t covered by existing tests. Consider adding unit/integration tests to verify both fullscreen and normal-mode behavior.
const hideTooltip = (): void => {

plugins/love-resources/src/components/RoomLanguageSelector.svelte:24

  • [nitpick] The imported isFullScreen value is not used in this file. Consider removing the import or wiring it into a conditional if you intend to adapt the selector for fullscreen mode.
import { updateSessionLanguage, isFullScreen } from '../utils'

plugins/love-resources/src/components/ControlBar.svelte:310

  • fullScreen is not defined or imported in this component, so the block never renders. You should import or derive the fullscreen flag (for example via a store or isFullScreen()) before using it.
{#if fullScreen}

packages/ui/src/components/TooltipInstance.svelte:69

  • The standard DOM API does not define document.fullscreen. To detect fullscreen mode, use document.fullscreenElement (or the appropriate vendor-prefixed API) instead of document.fullscreen.
const checkFS = (): boolean => (fullScreen && !document.fullscreen) || (!fullScreen && document.fullscreen)

packages/ui/src/components/Popup.svelte:32

  • Comparing popup.element (which is usually an HTMLElement) to the string 'full-centered' will always be true. Clarify which property indicates a centered fullscreen popup, or compare against the correct field (e.g. popup.options.position).
(fullScreen && document.fullscreen && popup.element !== 'full-centered') ||

@SasLord SasLord marked this pull request as draft May 16, 2025 09:59
@SasLord SasLord force-pushed the popups-in-fullscreen branch from 4e04683 to b85596c Compare May 16, 2025 10:16
@SasLord SasLord marked this pull request as ready for review May 16, 2025 10:17
@SasLord SasLord requested a review from Copilot May 16, 2025 10:20
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates popup and tooltip handling to improve full screen mode behavior. Key changes include:

  • Replacing legacy popup state management with direct usage of the new Popup component in ControlBar.svelte.
  • Enhancing TooltipInstance.svelte with a fullScreen flag and updated conditional logic for tooltip display.
  • Introducing a fullScreen prop and new filtering logic in Popup.svelte to control popup visibility.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File Description
plugins/love-resources/src/components/ControlBar.svelte Removed legacy popup methods and state in favor of the new Popup component and direct invocation via showPopup.
packages/ui/src/components/TooltipInstance.svelte Added a fullScreen prop and adjusted tooltip hide/show logic, including conditional SVG mask rendering.
packages/ui/src/components/Popup.svelte Added a fullScreen prop and refactored popup instance filtering logic with a new shouldDisplayPopup function.
Comments suppressed due to low confidence (1)

packages/ui/src/components/Popup.svelte:32

  • [nitpick] The conditional logic in shouldDisplayPopup is complex; consider refactoring or adding detailed comments to clarify the intent behind the fullScreen popup display conditions.
return ( (fullScreen && document.fullscreenElement != null && popup.element !== 'full-centered') || (!fullScreen && document.fullscreenElement != null && popup.element === 'full-centered') || (!fullScreen && document.fullscreenElement == null) )

@SasLord SasLord marked this pull request as draft May 16, 2025 10:21
@SasLord SasLord force-pushed the popups-in-fullscreen branch from b79a337 to 64d42e7 Compare May 16, 2025 11:07
@SasLord SasLord requested a review from Copilot May 16, 2025 11:08
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the UI components to support proper behavior of popups and tooltips when entering full screen mode. Key changes include:

  • Removal of the custom getPopup function and related state management in ControlBar.svelte with a simplified full-screen invocation.
  • Updates to TooltipInstance.svelte to conditionally hide tooltips based on full screen status.
  • Adjustments in Popup.svelte to control popup visibility through a new fullScreen flag and dedicated display logic.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.

File Description
plugins/love-resources/src/components/ControlBar.svelte Removed legacy popup handling and updated rendering to conditionally show Popup and TooltipInstance in full screen mode.
packages/ui/src/components/TooltipInstance.svelte Added fullScreen support with conditional rendering and tooltip visibility logic.
packages/ui/src/components/Popup.svelte Introduced a fullScreen property and a shouldDisplayPopup function to manage popup display logic.
Comments suppressed due to low confidence (1)

plugins/love-resources/src/components/ControlBar.svelte:311

  • Ensure that the removal of the previous popup state management does not break any other functionality. Verify that the new approach covers all required popup behaviors in full screen and non-full screen modes.
<Popup fullScreen />

@SasLord SasLord marked this pull request as ready for review May 16, 2025 11:11
SasLord added 7 commits May 19, 2025 15:56
Signed-off-by: Alexander Platov <[email protected]>
Signed-off-by: Alexander Platov <[email protected]>
Signed-off-by: Alexander Platov <[email protected]>
Signed-off-by: Alexander Platov <[email protected]>
Signed-off-by: Alexander Platov <[email protected]>
Signed-off-by: Alexander Platov <[email protected]>
Signed-off-by: Alexander Platov <[email protected]>
@SasLord SasLord force-pushed the popups-in-fullscreen branch from 42b0fc4 to 3ddf8b0 Compare May 19, 2025 08:56
@SasLord SasLord requested a review from Copilot May 19, 2025 08:58
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

Adds full-screen support for popups and tooltips by refactoring how they’re instantiated and rendered.

  • ControlBar now uses generic <Popup fullScreen/> and <TooltipInstance fullScreen/> instead of per-popup <PopupInstance>
  • TooltipInstance.svelte and Popup.svelte gain a fullScreen prop and logic to show/hide based on document.fullscreenElement
  • Overlay and mask rendering in TooltipInstance updated to respect full-screen state

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.

File Description
plugins/love-resources/src/components/ControlBar.svelte Replaced individual popup/tooltip instances with generic full-screen components
packages/ui/src/components/TooltipInstance.svelte Added fullScreen prop and centralized visibility logic
packages/ui/src/components/Popup.svelte Introduced fullScreen prop and updated popup filtering logic
Comments suppressed due to low confidence (2)

packages/ui/src/components/TooltipInstance.svelte:26

  • [nitpick] The new fullScreen prop introduces alternate rendering paths. Add unit or integration tests for both full-screen and normal modes to verify tooltip visibility and positioning behavior.
export let fullScreen: boolean = false

packages/ui/src/components/Popup.svelte:22

  • [nitpick] With the addition of fullScreen and shouldDisplayPopup logic, add tests covering popup visibility in full-screen and standard modes to prevent regressions.
export let fullScreen: boolean = false

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