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.

Sign up for GitHub

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

Jump to bottom

Trigger update when container resizes #28

Open
wants to merge 1 commit into
base: v5
Choose a base branch
from
Open

Trigger update when container resizes #28

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 18 additions & 2 deletions packages/module/src/LogViewer/LogViewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,8 @@ const LogViewerBase: React.FunctionComponent<LogViewerProps> = memo(
const [loading, setLoading] = useState(true);
const [listKey, setListKey] = useState(1);

const [, setEmptyState] = useState({});

/* Parse data every time it changes */
const parsedData = React.useMemo(() => parseConsoleOutput(data), [data]);

Expand All @@ -113,16 +115,27 @@ const LogViewerBase: React.FunctionComponent<LogViewerProps> = memo(
let resizeTimer = null as any;

useEffect(() => {
let observer : ResizeObserver | undefined = undefined;
if (containerRef && containerRef.current) {
window.addEventListener('resize', callbackResize);
observer = new ResizeObserver((event) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you check the event type here @mmuzikar? if event.type === 'scroll' then we don't do the callbackResize, otherwise we could use callbackResize.

Copy link
Author

Choose a reason for hiding this comment

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

Do you mean the event in the ResizeObserver? Because there is no type there. Or is this comment meant for the line above?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, I mean the event here, sorry I thought this is an HTML DOM event here.

Copy link

Choose a reason for hiding this comment

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

@DaoDaoNoCode I took another look at this and realized Drawer has an onResize callback. I wonder if somehow you could come at it from that angle instead, and somehow trigger the LogViewer to recalculate its layout imperatively when the drawer is resized. It would of course be nice to have a general solution where the LogViewer handles this itself, but perhaps that would be less hacky? I'm not sure how you would trigger it from outside the LogViewer, just a thought.

Copy link

Choose a reason for hiding this comment

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

@DaoDaoNoCode I took another look at this and realized Drawer has an onResize callback. I wonder if somehow you could come at it from that angle instead, and somehow trigger the LogViewer to recalculate its layout imperatively when the drawer is resized. It would of course be nice to have a general solution where the LogViewer handles this itself, but perhaps that would be less hacky? I'm not sure how you would trigger it from outside the LogViewer, just a thought.

@mturley I tried using onResize callback on drawer in this PR: opendatahub-io/odh-dashboard#2184
Discussion : opendatahub-io/odh-dashboard#2184 (comment)
But it didn't work as expected. onResize it was not recalcualting the lineHeight. Line height recalculation was happening when the window resizes.

GIF:
Screencast from 2023-11-20 17-55-51.webm

Copy link
Author

Choose a reason for hiding this comment

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

I updated my PR with the changes you requested, I am not sure I'm following the conversation as I am no React expert, if you have a better idea on how to fix this issue, I can close this and let you do it the right way :)

setEmptyState({});
Copy link
Author

Choose a reason for hiding this comment

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

I tried calling callbackResize here put it interfered with scrolling behavior (resize is called even for scrolling).

Copy link

Choose a reason for hiding this comment

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

[curious-question]: Are we using setEmptyState as a way to rerender the component, could be possible to use setLoading(false) as well? would that work?

Copy link
Author

Choose a reason for hiding this comment

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

setLoading(false): Same behavior as before the fix
setLoading(true) or setResizing(true): the log container is empty

});
observer.observe(containerRef.current);
setLoading(false);
createDummyElements();
ansiUp.resetStyles();
}
return () => window.removeEventListener('resize', callbackResize);
return () => {
window.removeEventListener('resize', callbackResize);
observer?.disconnect();
}
}, [containerRef.current]);

const callbackResize = () => {
const callbackResize = (event : UIEvent) => {
if (event.type === 'scroll') {
return;
}
if (!resizing) {
setResizing(true);
}
Expand Down Expand Up @@ -168,6 +181,9 @@ const LogViewerBase: React.FunctionComponent<LogViewerProps> = memo(
}, [parsedData, scrollToRow]);

const createDummyElements = () => {
if (!(containerRef && containerRef.current)) {
return;
}
Comment on lines +184 to +186
Copy link
Author

Choose a reason for hiding this comment

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

To prevent manipulation of DOM when the timer callback happens after the container was closed.

// create dummy elements
const dummyIndex = document.createElement('span');
dummyIndex.className = css(styles.logViewerIndex);
Expand Down