Bug 2033065: fix pod log height issue #11361
Conversation
openshift-ci
bot
added
bugzilla/severity-medium
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@abhinandan13jan: An error was encountered querying GitHub for users with public email (spathak@redhat.com) for bug 2033065 on the Bugzilla server at https://bugzilla.redhat.com. No known errors were detected, please see the full error message for details. Full error message.
non-200 OK status code: 403 Forbidden body: "{\n \"documentation_url\": \"https://docs.github.com/en/free-pro-team@latest/rest/overview/resources-in-the-rest-api#secondary-rate-limits\",\n \"message\": \"You have exceeded a secondary rate limit. Please wait a few minutes before you try again.\"\n}\n"
Please contact an administrator to resolve this issue, then request a bug refresh with In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Does PF have a collapsible toolbar where there is an overflow menu for small screens? |
|
@spadgett Yes, there is an |
|
Should we try a collapsible here? @spadgett |
abhinandan13jan
force-pushed
the
fix-pod-log-height
branch
from
169f057
to
fe54d1a
Compare
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@spadgett This is how it looks in the OverflowMenu. Please let me know whether to proceed with this? |
|
@abhinandan13jan Thank you, this looks like a better approach to me 👍 Adding @megan-hall for UX input on the items and position of the overflow menu. |
I wonder if we should keep the overflow menu to the right of search, and keep the items in the list in the order that they appear left to right? I have to admit it feels a little strange to put the drop downs in the overflow. Is there away to keep those two items out but still have them stack in a more responsive way? |
|
Is something like this possible?
|
|
Hello @megan-hall, there are two issues around the design requested.
The search widget expands on input and it is not ideal to fit it in a row of three elements.
We can implement either of the following.1. Add the dropdown collapse icon in the middle(end of row) the first row.
2. Rearrange and have it on the right of second row 3. Put it on the left of either of the rows. Please left me know how to proceed? cc: @spadgett @invincibleJai |
abhinandan13jan
force-pushed
the
fix-pod-log-height
branch
from
fe54d1a
to
df60bfb
Compare
openshift-ci
bot
added
the
kind/i18n
We should raise this upstream in PatternFly. The overflow menu often is used for mobile, so placement seems important. cc @jcaianirh |
|
Raised PF Bug patternfly/patternfly-react#7398 |
Though Option 2 is not so "pretty" it does behave in the way I think users would expect. As the page collapses, the functionality to the right of search collapses into the icon and consistently appears to the right of the search input field. |
abhinandan13jan
force-pushed
the
fix-pod-log-height
branch
from
df60bfb
to
7cb8756
Compare
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
Updated PR to use option 2. Currently looks like the following @megan-hall |
|
/assign @TheRealJon Jon, can you help review since you're familiar with the log components? |
There was a problem hiding this comment.
I did some testing on this and found a few issues. It seems like there's been a lot of back and forth on the design of this, but the responsive behavior of this solution is a little weird. Here's a screen capture of the testing I did:
- The actions are no longer right-aligned.
- They begin wrapping right before the breakpoint where they collapse into an overflow menu. We probably need to use a '2xl' breakpoint to make these actions collapse much sooner.
- The search bar wraps to a second line at smaller widths, which takes up valuable log viewer screen space and just doesn't quite look right. Even though it's an improvement over the previous behavior, IMO, it could be even better.
I'm wondering if this is an opportunity to use the PatternFly Toolbar and specifically the ToolbarToggleGroup component. Has that come up as an option during previous discussions? See an example here: https://www.patternfly.org/v4/components/toolbar#with-filters
This would allow us to collapse the search, container dropdown, and current/previous log dropdown at smaller widths:
| const overflowActions = [ | ||
| logURLLink, | ||
| downloadLink, | ||
| wrapLoglinesCheckbox, | ||
| ...(shouldShowDebugAction ? [debugAction] : []), | ||
| ...podLogExternalLinks, | ||
| ...(screenfull.enabled ? [fullscreenButton] : []), | ||
| ]; |
There was a problem hiding this comment.
Taking a second look at this, we probably need to retain the original order of the log controls, unless it was otherwise specified in previous conversation:
| const overflowActions = [ | |
| logURLLink, | |
| downloadLink, | |
| wrapLoglinesCheckbox, | |
| ...(shouldShowDebugAction ? [debugAction] : []), | |
| ...podLogExternalLinks, | |
| ...(screenfull.enabled ? [fullscreenButton] : []), | |
| ]; | |
| const overflowActions = [ | |
| ...(shouldShowDebugAction ? [debugAction] : []), | |
| ...podLogExternalLinks, | |
| wrapLoglinesCheckbox, | |
| logURLLink, | |
| downloadLink, | |
| ...(screenfull.enabled ? [fullscreenButton] : []), | |
| ]; |
There was a problem hiding this comment.
updated, now it looks like the description screenshot
| if (isMobile) { | ||
| height !== LOG_MOBILE_HEIGHT && setHeight(LOG_MOBILE_HEIGHT); | ||
| } else { | ||
| height !== LOG_FULLSCREEN_HEIGHT && setHeight(LOG_FULLSCREEN_HEIGHT); | ||
| } |
There was a problem hiding this comment.
This isn't ideal for a couple of reasons:
useIsMobileis only evaluated on mount, so this will not update on resize. The one scenario where I could see this impacting UX is when rotating a mobile or tablet between portrait and landscape orientation.- A static 500px height could be too tall on some mobile screens, which might cause scrolling issues.
It might be worth investigating a more responsive solution.
There was a problem hiding this comment.
I understand. But maybe this is the best bet we have right now as the height if not explicitly specified the content issue remains. Hence it was discussed with UX for the same. I've changed the height to 300px. Do let me know what is to be done here?
Without specific height:
| /> | ||
| </OverflowMenuItem> | ||
| </OverflowMenuGroup> | ||
| <OverflowMenuGroup className="co-toolbar__group co-toolbar__group--right"> |
There was a problem hiding this comment.
Ideally, we could remove these classes that override PF styles, but I'm not sure what impact that will have.
| <OverflowMenuGroup className="co-toolbar__group co-toolbar__group--right"> | |
| <OverflowMenuGroup> |
| @@ -148,6 +159,45 @@ const showDebugAction = (pod: PodKind, containerName: string) => { | |||
| return !containerStatus?.state?.running || !containerStatus.ready; | |||
| }; | |||
|
|
|||
| const getPodLogLinkUrl = (podLogLink, templateValues) => { | |||
There was a problem hiding this comment.
Can you add some types here, please.
| return replaceVariables(hrefTemplate, templateValues); | ||
| }; | ||
|
|
||
| const getPodLogExternalLinks = (podLogLinks, templateValues) => |
There was a problem hiding this comment.
Can you also add some types here please. Thanks
abhinandan13jan
force-pushed
the
fix-pod-log-height
branch
from
2d8055d
to
dc8eb80
Compare
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
abhinandan13jan
force-pushed
the
fix-pod-log-height
branch
from
dc8eb80
to
8dbd34f
Compare
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
1 similar comment
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
abhinandan13jan
force-pushed
the
fix-pod-log-height
branch
from
8dbd34f
to
89321c1
Compare
|
@abhinandan13jan: The following test failed, say
Full PR test history. Your PR dashboard. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
|
LGTM! Thanks for all the collaboration on this! |
There was a problem hiding this comment.
I've talked with @megan-hall about this and we came to a consensus that we should just try to match the behaviors of the PatternFly toolbar example. So, the "actions" (view raw, download, toggle wrap lines, etc.) should collapse into a kebab dropdown at the 2xl breakpoint, then the search, container, and log type dropdowns should collapse into a "filter" dropdown at the lg breakpoint. Here is a rough example that I worked up using the ToolbarToggleGroup and OverflowMenu components. This is not fully hashed out or tested, but it should get you most of the way there:
<Toolbar>
<ToolbarContent>
<ToolbarItem>{showStatus()}</ToolbarItem>
<ToolbarToggleGroup toggleIcon={<FilterIcon />} breakpoint="lg">
<ToolbarItem variant="search-filter">
<LogViewerSearch
onFocus={() => {
if (status === STREAM_ACTIVE) {
toggleStreaming();
}
}}
placeholder="Search"
/>
</ToolbarItem>
{dropdown && <ToolbarItem>{dropdown}</ToolbarItem>}
<ToolbarItem>{logTypeSelect(!hasPreviousLog)}</ToolbarItem>
</ToolbarToggleGroup>
<ToolbarItem>
<OverflowMenu breakpoint="2xl">
<OverflowMenuContent>
{logActions.map((action, index) => {
return <ToolbarItem key={`log-control-item-${index}`}>{action}</ToolbarItem>;
})}
</OverflowMenuContent>
<OverflowMenuControl>
<Dropdown
toggle={<KebabToggle onToggle={onToggleOverflowDropdown} />}
isOpen={isOverflowDropdownOpen}
isPlain
dropdownItems={logActions.map((action, index) => {
return (
<DropdownItem key={`log-control-overflow-item-${index}`}>{action}</DropdownItem>
);
})}
/>
</OverflowMenuControl>
</OverflowMenu>
</ToolbarItem>
</ToolbarContent>
</Toolbar>| type TemplateValues = { | ||
| namespace?: string; | ||
| resourceName: string; | ||
| resourceUID: string; | ||
| containerName: string; | ||
| resourceNamespace: string; | ||
| resourceNamespaceUID: string; | ||
| podLabels: string; | ||
| }; | ||
|
|
||
| type Line = { length: number }; | ||
|
|
There was a problem hiding this comment.
| type TemplateValues = { | |
| namespace?: string; | |
| resourceName: string; | |
| resourceUID: string; | |
| containerName: string; | |
| resourceNamespace: string; | |
| resourceNamespaceUID: string; | |
| podLabels: string; | |
| }; | |
| type Line = { length: number }; | |
| type TemplateValues = { [key: string]: string }; |
| @@ -105,14 +126,18 @@ const getResourceLogURL = ( | |||
| }); | |||
| }; | |||
|
|
|||
| const HeaderBanner = ({ lines }) => { | |||
| const HeaderBanner: React.FC<{ lines: Line[] }> = ({ lines }) => { | |||
There was a problem hiding this comment.
| const HeaderBanner: React.FC<{ lines: Line[] }> = ({ lines }) => { | |
| const HeaderBanner: React.FC<{ lines: string[] }> = ({ lines }) => { |
| <Toolbar | ||
| id="toolbar-with-filter" | ||
| className="pf-m-toggle-group-container" | ||
| collapseListedFiltersBreakpoint="lg" | ||
| clearAllFilters={() => {}} | ||
| > |
There was a problem hiding this comment.
Do we need these props? IIRC, it worked as expected without them
| <Toolbar | |
| id="toolbar-with-filter" | |
| className="pf-m-toggle-group-container" | |
| collapseListedFiltersBreakpoint="lg" | |
| clearAllFilters={() => {}} | |
| > | |
| <Toolbar> |
|
I opened a PatternFly issue for the ToolbarToggleGroup component to address the problem you are seeing where you can't use multiple ToolbarToggleGroups in a single Toolbar: patternfly/patternfly-react#7590 |
|
/retest |
|
Hey @abhinandan13jan, on the desktop this PR changes the order of the input fields, that's maybe fine. 1. But couldn't we span and right align the last group (Wrap lines ... Expand) ??Before: Now: Since this is a flex layout this should be possible with For sure, this must be added to a specific breakpoint, but the different Toolbar components has props for that, or? 2. When reducing the window size to a "medium width" I got a more action (3 dots) which jumps around when clicking it:jumping-more-button.mp43. And then I'm personally also confused when reducing the width, even more, why do we have a filter and a more (3 dots) button which does the same?same-function.mp44. And finally, there is a fix height, as someone mentioned above. It makes sense for me to have a min-height to force a scrollbar, but a fix or max-height doesn't allow the user to use the full height of his phone.max-height.mp4I would recommend rebasing this PR and then taking a fresh look on how we can improve all this. |
|
@jerolimov - just providing my two cents based on your comments above. @TheRealJon keep me honest on my comments below based on our chat on Friday. 1. But couldn't we span and right align the last group (Wrap lines ... Expand) ??Agreed - we should keep wrap lines, raw, download and expand content functionality aligned on the right. 3. And then I'm personally also confused when reducing the width, even more, why do we have a filter and a more (3 dots) button which does the same?This is what PF recommends. |
|
I would disagree on your first point @jerolimov (right aligning the actions). We would need to override PatternFly to do this, which introduces unnecessary complexity. IMO, we should just align to the PatternFly Toolbar pattern. As far as point 3, there is a bug in PatternFly that is causing both the kebab and filter dropdowns to have the same content. I reported this to PF, but for now we could work around this by using a combination of the |
|
We also should include the search input in the ToolbarToggleGroup so that all of the toolbar items get collapsed at smaller widths. We can see this pattern in the PatternFly Toolbar component examples: https://www.patternfly.org/v4/components/toolbar/react/with-filters/ |
PF provides an alignment prop for ToolbarGroup, which support breakpoints out of the box. So I would not call it “override PatternFly”, it's just using an existing feature explicit provided by their API. See https://codesandbox.io/s/toolbar-example-4mx339?file=/index.js |
PF includes props that allow us to do this easily, so scratch that. |
|
I've been experimenting with the PF Toolbar today and testing edge cases for our proposed solution. It might be out of scope, but there is an issue with listing all of the log actions horizontally above the log viewer. The length of the log actions list is arbitrary. We provide an API (ConsoleExternalLogLink) that allows users to add an arbitrary number of links, each with arbitrary display text. This quickly causes issues with responsiveness, especially if the container name is long and the "Debug container" action is present. I don't know that we need to worry about handling the case of multiple ConsoleExternalLogLinks, since that is likely a rare use case, but even one external link with relatively long display text will cause responsiveness issues near the 2xl breakpoint. Here are some gifs of my experimentation: Near the 2XL (1450px) breakpoint, the labels start wrapping, and the list overflows: Near the LG (992px) breakpoint, things look nice, although the search input shrinks a bit with longer container names: |
|
Closing this out as there are higher priority stories on HAC-DEV & HAC-BS side |
|
@abhinandan13jan: This pull request references Bugzilla bug 2033065. The bug has been updated to no longer refer to the pull request using the external bug tracker. All external bug links have been closed. The bug has been moved to the NEW state. In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
Issue
Addresses https://bugzilla.redhat.com/show_bug.cgi?id=2033065 / https://issues.redhat.com/browse/OCPBUGSM-38515
Also organizes the LogHeader in an OverflowMenu
Description
Log viewer toolbar does not wrap correctly. The height of the LogView is always set to
100%. This causes the header elements to align undesirably and the logs are not readable on mobile screen.Fix Screenshots
Tests
none
Browser conformance
Chrome
/cherrypick release-4.9
/cherrypick release-4.10