Skip to content
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

frontend: LogViewer: Fix log not expanding on full screen #2650

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

vyncent-t
Copy link
Contributor

@vyncent-t vyncent-t commented Dec 9, 2024

fixes issue #2640

Description

This PR fixes an issue with the LogViewer component where log lines did not expand to the full width when in full-screen mode. This issue impacts user experience, especially for users relying on the full-screen view to read long log lines.

Key Changes

  1. Full-Width Expansion:
    • Updated the LogViewer component's logic allowing render handlers to run resizes to ensure log lines expand to utilize the full width in full-screen mode.

Steps to Test

  1. Open the LogViewer component in the pods details view.

  2. Switch to full-screen mode.

  3. Verify:

    • Log lines expand to the full width of the screen in full-screen mode.
    • Log lines remain readable and aligned correctly in both standard and full-screen modes.
  4. Test on various screen sizes to ensure responsiveness and compatibility.

@vyncent-t vyncent-t added the regression Bugs for things that used to work in previous releases. label Dec 9, 2024
@vyncent-t vyncent-t self-assigned this Dec 9, 2024
@vyncent-t vyncent-t force-pushed the fix-full-screen-log-render branch 2 times, most recently from 60d9f1b to 0c709a4 Compare December 9, 2024 22:42
@vyncent-t vyncent-t requested a review from sniok December 9, 2024 22:56
@vyncent-t vyncent-t changed the title WIP: frontend: LogViwer: Fix log not expanding on full screen frontend: LogViwer: Fix log not expanding on full screen Dec 9, 2024
@vyncent-t vyncent-t marked this pull request as ready for review December 9, 2024 22:56
@dosubot dosubot bot added the size:S This PR changes 10-29 lines, ignoring generated files. label Dec 9, 2024
@vyncent-t
Copy link
Contributor Author

note: I have tried setting different widths to take up the full size of the container, this would only resize on save after the full screen mode is already expanded and would not work if starting from the beginning (goes to details, opens log viewer, click full screen, would still be half size)

the xterm fitAddonRef.current!.fit(); is the main fix for setting the sizes, I also tried looking into ResizeObserver but I think that would be too much for performance.

@sniok
Copy link
Contributor

sniok commented Dec 10, 2024

would calling fitAddonRef.current!.fit(); inside onFullScreenToggled callback on Dialog component work? This current change seems overly complicated, there's a new state variable with an unusual setter that seemingly triggers the effect that sets up the whole terminal from scratch.

@joaquimrocha joaquimrocha added this to the v0.27.0 milestone Dec 11, 2024
@vyncent-t
Copy link
Contributor Author

vyncent-t commented Dec 11, 2024

@sniok @joaquimrocha

I have tried the following and it does not seem to not fix the full screen issue, the console log prints but the fit method does not resize anything, I tried to call it inside the onFullScreenToggled entirely and I also tried creating a stand alone function and passing that in and had the same result, all three ways still print but the calling of the fitAddonRef.current?.fit() does not work, I saw this trend of style changes (settings sx width and minWidth and also different flex displays ) and fitAddonRef.current?.fit() not rendering unless the screen is already maximized and I'm opening the changes live and thought maybe it just needed to rerender from the useEffect

<Dialog title={title} withFullScreen onClose={onClose} onFullScreenToggled={(isFullScreen: boolean) => { if (isFullScreen) { fitAddonRef.current?.fit(); console.log("REFITS") } }} {...other}>


image


image

--

also tried inserting some new instances to maybe try that too but nothing


    fitAddonRef.current = new FitAddon();

    if (isFullScreen) {
      fitAddonRef.current?.fit();
      console.log("REFITS")
    }
  }

@vyncent-t
Copy link
Contributor Author

vyncent-t commented Dec 11, 2024

using the timer fix from this part uses in the project @sniok

<Dialog title={title} onFullScreenToggled={() => { setTimeout(() => { fitAddonRef.current!.fit(); }, 1); }} withFullScreen onClose={onClose} {...other} >

@vyncent-t vyncent-t force-pushed the fix-full-screen-log-render branch from 0c709a4 to 14bb07b Compare December 11, 2024 17:11
@vyncent-t
Copy link
Contributor Author

need i18n missing words from #2660

@skoeva skoeva changed the title frontend: LogViwer: Fix log not expanding on full screen frontend: LogViewer: Fix log not expanding on full screen Dec 11, 2024
Copy link
Contributor

@skoeva skoeva left a comment

Choose a reason for hiding this comment

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

The i18n failure should be fixed by rebasing, otherwise LGTM

@joaquimrocha joaquimrocha merged commit 2758f7b into main Dec 11, 2024
16 of 18 checks passed
@joaquimrocha joaquimrocha deleted the fix-full-screen-log-render branch December 11, 2024 22:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
regression Bugs for things that used to work in previous releases. size:S This PR changes 10-29 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

4 participants