-
Notifications
You must be signed in to change notification settings - Fork 609
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
Bug 2033065: fix pod log height issue #11361
Bug 2033065: fix pod log height issue #11361
Conversation
@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 |
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? |
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 |
fe54d1a
to
df60bfb
Compare
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isn't ideal for a couple of reasons:
useIsMobile
is 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/> | ||
</OverflowMenuItem> | ||
</OverflowMenuGroup> | ||
<OverflowMenuGroup className="co-toolbar__group co-toolbar__group--right"> |
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.
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> |
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.
changed
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add some types here, please.
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.
updated
return replaceVariables(hrefTemplate, templateValues); | ||
}; | ||
|
||
const getPodLogExternalLinks = (podLogLinks, templateValues) => |
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.
Can you also add some types here please. Thanks
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.
updated
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. |
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. |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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) ??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. |
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.
Using Toolbar is the right direction, but we should improve the UI based on different breakpoints even more. Let me know if I should take a deeper look into the Toolbar components.
@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. |
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