-
Notifications
You must be signed in to change notification settings - Fork 14.5k
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
Add full screen view for logs #45183
base: main
Are you sure you want to change the base?
Conversation
|
||
<Dialog.CloseTrigger /> | ||
|
||
<Dialog.Body> |
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.
We should keep the try selector and wrap/unwrap button in the modal
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.
Thanks, I suppose the entire content from can become a component like <LogComponent>
and then show FullScreen
based on the present inside the dialog or not like below. Are there any other ways to do this idiomatic in react without a separate component?
<Box>
<LogComponent showFullScreenButton />
<Dialog>
<LogComponent />
</Dialog>
</Box>
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.
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.
+1 for keeping the header. It's taking really few real estate in the page and allowing to navigate without having to close and then open again the full screen mode.
That's also consistant with how we build the graph,grid, etc... views. (there's a header). Also maybe the Task id too would be helpful. (for the grid graph etc... we have the dag_id). I can easily imagine someone losing track of what logs he is looking at after a while reading in the full screen page. Let's imagine we have multiple opened tabs like this at the same time, it's difficult to navigate and remember about.
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.
Thanks @bbovenzi and @pierrejeambrun for the feedback. It also makes it consistent with other pages if this is the intended design. I will wait for @bbovenzi comment to see if there is any way to do this in react since without a component or if a component is the best way.
Related #44663 (comment)
It will be useful to have logs open in full screen view to read the logs. Currently the tab and task details are still present on scroll taking more space. Legacy logs page had more screen space which was useful for our users internally. The implementation is simple that uses dialog to display the same code block with wrap and other styles applied in full screen mode.
Notes for reviewer and self :