Convert Log Viewer to PF Log Viewer #2184

ppadti · 2023-11-20T12:11:50Z

Description

This PR aims to change the existing Log viewer to PF Log viewer. Also this PR includes

scroll up to pause feature.
Find/search feature in the log viewer toolbar.
Support for a full-screen log viewer (expand functionality).

UX Design: https://www.figma.com/proto/rJjOtl3nxpW6BAcl0MZr4y/Pipeline-log-viewer?page-id=62%3A1895&type=design&node-id=1075-5045&viewport=1226%2C-14994%2C0.33&t=BbsTwCedaJMiJaRo-1&scaling=min-zoom&starting-point-node-id=1075%3A5045&show-proto-sidebar=1&mode=design

Gifs:

Screencast.from.2024-01-16.22-27-04.webm

Screencast.from.2024-01-16.22-28-20.webm

Screencast.from.2024-01-16.16-31-09.webm

Screenshots:

When the drawer is at its default width.

Log viewer when Drawer is at minSize.

When search bar is expanded

Log viewer when "No logs available"

How Has This Been Tested?

Upload a Pipeline & create runs
Select a node & go to the logs tab
View logs for containers

Test Impact

Request review criteria:

Self checklist (all need to be checked):

The developer has manually tested the changes and verified that the changes work
Commits have been squashed into descriptive, self-contained units of work (e.g. 'WIP' and 'Implements feedback' style messages have been removed)
Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).
The developer has added tests or explained why testing cannot be added (unit tests & storybook for related changes)

If you have UI changes:

Included any necessary screenshots or gifs if it was a UI change.
Included tags to the UX team if it was a UI/UX change (find relevant UX in the SMEs section).

After the PR is posted & before it merges:

The developer has tested their solution on a cluster by using the image produced by the PR to main

...d/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerRightTabs.tsx

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx

ppadti · 2023-11-20T12:34:22Z

This solution has text overlapping issue, when drawer size gets smaller. We can avoid this problem by increasing the minSize of Drawer. Please share some suggestions or ideas you might have.

Screencast.from.2023-11-20.17-55-51.webm

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx

frontend/src/concepts/dashboard/DashboardLogViewer.tsx

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx

andrewballantyne

This needs to be rebased against the PF5 changes.

The resizing effort is alright... I think we may want to find a nicer way to manage it in the long run because it definitely looks very hacky right not.

I think we need to probably discuss the whole need for the Log tab to be selected. This seems like a technical issue you're having and trying to bend the UX to fit it -- where I assume we can figure out a proper technical solution to avoid it.

frontend/src/concepts/dashboard/DashboardLogViewer.tsx

...rc/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerRightContent.tsx

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx

yih-wang · 2023-11-30T07:18:55Z

Hi @ppadti , what is the current minSize of the drawer?

ppadti · 2023-11-30T07:47:44Z

Hi @ppadti , what is the current minSize of the drawer?

current minSize of the drawer is 400px.

mturley · 2023-12-05T19:57:11Z

@ppadti , I've opened a PR against this PR's branch to work around the LogViewer rendering issue and reset the default tab back to Input / Output. Please review: ppadti#1

…ng issue (#1) * Switch default tab back to input/output Signed-off-by: Mike Turley <[email protected]> * Conditionally render only the active tab, remove now-unneccessary tabContentProps function Signed-off-by: Mike Turley <[email protected]> --------- Signed-off-by: Mike Turley <[email protected]>

manaswinidas · 2024-01-08T20:48:11Z

Everything looks good but the logs aren't scrolling to the bottom for me.

Screen.Recording.2024-01-09.at.2.11.19.AM.mov

ppadti · 2024-01-09T11:52:12Z

Everything looks good but the logs aren't scrolling to the bottom for me.

Screen.Recording.2024-01-09.at.2.11.19.AM.mov

I am not able to reproduce this issue. @DaoDaoNoCode could you please check whether this issue exist, while reviewing the PR.

yih-wang · 2024-01-09T12:03:45Z

Hi @ppadti, the updates look good! I remember when seeing your demo during our meeting, the search box remains the same width both for the default status and focus status. Is that been changed or do I get a wrong memory here?

Another question is do the spaces between the components follow the standard pattern? I feel like these two spaces are not equal to each other. But that could be my illusion.

ppadti · 2024-01-10T15:57:46Z

Hi @ppadti, the updates look good! I remember when seeing your demo during our meeting, the search box remains the same width both for the default status and focus status. Is that been changed or do I get a wrong memory here?

Another question is do the spaces between the components follow the standard pattern? I feel like these two spaces are not equal to each other. But that could be my illusion.

@yih-wang I have updated the maxwidth for search item to 250px, when it is expanded. I didn't quiet get what default status means. Is it only the search icon that we display without searchInput?

yeah the spaces are not equal. Because the steps, search and pause/resume item stays in one ToolabarToggelGroup which is aligned to the left of the drawer and download dropdown and kebab menu stays in another ToolabarToggelGroup , which aligns right of the drawer, when the drawer is resized to large. you want me make them have equal space when the drawer is at minSize?

yih-wang · 2024-01-12T03:02:35Z

@ppadti

...the search box remains the same width both for the default status and focus status...

Here I mean when the search box expands, the width of what the user defaults to see (before typing anything in the search box) or what the user sees after typing something in the search box (search box with 1/7 ^⌄ in it).

you want me make them have equal space when the drawer is at minSize?

Yes I'm wondering whether we could make all the spaces 16px at minSize, especially for the layout below.

DaoDaoNoCode · 2024-01-12T16:37:30Z

I checked the current behavior, looks like the clear button is clearing the search field input but clearing the search result.
Reproduce: Click search -> input something -> click on 'x' button to close search -> open it again

manaswinidas · 2024-01-15T06:58:29Z

@DaoDaoNoCode This is the issue I mentioned/demoed in the last office hours call. This is a known issue. @ppadti has created a PF issue for this.

ppadti · 2024-01-15T07:10:44Z

@DaoDaoNoCode This is the issue I mentioned/demoed in the last office hours call. This is a known issue. @ppadti has created a PF issue for this.

PF Issue: patternfly/react-log-viewer#56
Also there is another issue needs to be addressed to get text overlapping things resolved. It's metioned here: #2184 (comment)

ppadti · 2024-01-16T11:08:29Z

@ppadti

...the search box remains the same width both for the default status and focus status...

Here I mean when the search box expands, the width of what the user defaults to see (before typing anything in the search box) or what the user sees after typing something in the search box (search box with 1/7 ^⌄ in it).

you want me make them have equal space when the drawer is at minSize?

Yes I'm wondering whether we could make all the spaces 16px at minSize, especially for the layout below.

@yih-wang I have updated the spaces between toolbar items as we discussed. Let me know if this alignment and spacing is fine.
minSize: 500px
GIF:
Screencast from 2024-01-16 16-31-09.webm

...rc/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerRightContent.tsx

...d/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerRightTabs.tsx

...end/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/DownloadDropdown.tsx

manaswinidas · 2024-01-16T16:24:29Z

@ppadti please update the GIFs and screenshots in the PR description too.

Co-authored-by: Mike Turley <[email protected]>

Gkrumbach07 · 2024-01-16T21:46:02Z

anything not working as expected is a PF bug (drawer resizing issue, highlighted text). everything else is working as expected.

needs UX approval and advisor

/lgtm

mturley

LGTM aside from the PF issues identified here which can probably be considered out of scope of this PR as long as we have issues to follow up on them.

...rc/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerRightContent.tsx

Changes have been addressed.

openshift-ci · 2024-01-17T06:35:21Z

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: manaswinidas, mturley

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

openshift-ci bot added the do-not-merge/work-in-progress This PR is in WIP state label Nov 20, 2023

openshift-ci bot requested review from DaoDaoNoCode and pnaik1 November 20, 2023 12:11

ppadti commented Nov 20, 2023

View reviewed changes

...d/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerRightTabs.tsx Outdated Show resolved Hide resolved

ppadti commented Nov 20, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx Outdated Show resolved Hide resolved

manaswinidas requested changes Nov 20, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx Outdated Show resolved Hide resolved

frontend/src/concepts/dashboard/DashboardLogViewer.tsx Outdated Show resolved Hide resolved

openshift-ci bot assigned manaswinidas Nov 20, 2023

manaswinidas requested changes Nov 20, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx Outdated Show resolved Hide resolved

openshift-merge-robot added the needs-rebase PR needs to be rebased label Nov 23, 2023

andrewballantyne previously requested changes Nov 24, 2023

View reviewed changes

openshift-ci bot assigned andrewballantyne Nov 24, 2023

manaswinidas linked an issue Nov 27, 2023 that may be closed by this pull request

[Feature Request]: Convert Log Viewer to use PF Log Viewer #1738

Closed

ppadti force-pushed the issue-1738 branch from 3ca8816 to b5712c9 Compare November 27, 2023 11:25

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Nov 27, 2023

ppadti force-pushed the issue-1738 branch from b5712c9 to b777832 Compare November 27, 2023 11:48

manaswinidas requested changes Nov 28, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx Show resolved Hide resolved

ppadti force-pushed the issue-1738 branch from b777832 to e042cc0 Compare November 29, 2023 11:47

ppadti commented Nov 29, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx Show resolved Hide resolved

ppadti commented Nov 29, 2023

View reviewed changes

frontend/src/concepts/pipelines/content/pipelinesDetails/pipelineRun/runLogs/LogsTab.tsx Outdated Show resolved Hide resolved

ppadti force-pushed the issue-1738 branch from e042cc0 to 6b5feda Compare November 30, 2023 07:45

openshift-merge-robot added the needs-rebase PR needs to be rebased label Dec 6, 2023

ppadti force-pushed the issue-1738 branch from 9c0581a to a1fd8cc Compare December 7, 2023 10:30

openshift-merge-robot removed the needs-rebase PR needs to be rebased label Dec 7, 2023

ppadti force-pushed the issue-1738 branch from 173a584 to 0a2d35b Compare January 8, 2024 12:22

ppadti mentioned this pull request Jan 15, 2024

Trigger update when container resizes patternfly/react-log-viewer#28

Open

ppadti force-pushed the issue-1738 branch from 0a2d35b to f590dc6 Compare January 16, 2024 14:19

Gkrumbach07 reviewed Jan 16, 2024

View reviewed changes

ppadti force-pushed the issue-1738 branch 2 times, most recently from 870e0bb to 58d0493 Compare January 16, 2024 18:43

Convert Log Viewer to PF Log Viewer

0402c5b

Co-authored-by: Mike Turley <[email protected]>

ppadti force-pushed the issue-1738 branch from 58d0493 to 0402c5b Compare January 16, 2024 20:07

openshift-ci bot assigned Gkrumbach07 Jan 16, 2024

openshift-ci bot added the lgtm label Jan 16, 2024

mturley approved these changes Jan 16, 2024

View reviewed changes

...rc/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerRightContent.tsx Show resolved Hide resolved

openshift-ci bot assigned mturley Jan 16, 2024

manaswinidas reviewed Jan 17, 2024

View reviewed changes

...rc/concepts/pipelines/content/pipelinesDetails/pipelineRun/PipelineRunDrawerRightContent.tsx Show resolved Hide resolved

manaswinidas added the approved label Jan 17, 2024

manaswinidas approved these changes Jan 17, 2024

View reviewed changes

openshift-merge-bot bot merged commit 0441a89 into opendatahub-io:f/pipelines-enhancement Jan 17, 2024
4 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert Log Viewer to PF Log Viewer #2184

Convert Log Viewer to PF Log Viewer #2184

ppadti commented Nov 20, 2023 •

edited

Loading

ppadti commented Nov 20, 2023

andrewballantyne left a comment

yih-wang commented Nov 30, 2023

ppadti commented Nov 30, 2023

mturley commented Dec 5, 2023

manaswinidas commented Jan 8, 2024

ppadti commented Jan 9, 2024

yih-wang commented Jan 9, 2024

ppadti commented Jan 10, 2024

yih-wang commented Jan 12, 2024

DaoDaoNoCode commented Jan 12, 2024

manaswinidas commented Jan 15, 2024

ppadti commented Jan 15, 2024 •

edited

Loading

ppadti commented Jan 16, 2024 •

edited

Loading

manaswinidas commented Jan 16, 2024 •

edited

Loading

Gkrumbach07 commented Jan 16, 2024 •

edited

Loading

mturley left a comment

openshift-ci bot commented Jan 17, 2024

Convert Log Viewer to PF Log Viewer #2184

Convert Log Viewer to PF Log Viewer #2184

Conversation

ppadti commented Nov 20, 2023 • edited Loading

Description

How Has This Been Tested?

Test Impact

Request review criteria:

ppadti commented Nov 20, 2023

andrewballantyne left a comment

Choose a reason for hiding this comment

yih-wang commented Nov 30, 2023

ppadti commented Nov 30, 2023

mturley commented Dec 5, 2023

manaswinidas commented Jan 8, 2024

ppadti commented Jan 9, 2024

yih-wang commented Jan 9, 2024

ppadti commented Jan 10, 2024

yih-wang commented Jan 12, 2024

DaoDaoNoCode commented Jan 12, 2024

manaswinidas commented Jan 15, 2024

ppadti commented Jan 15, 2024 • edited Loading

ppadti commented Jan 16, 2024 • edited Loading

manaswinidas commented Jan 16, 2024 • edited Loading

Gkrumbach07 commented Jan 16, 2024 • edited Loading

mturley left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 17, 2024

ppadti commented Nov 20, 2023 •

edited

Loading

ppadti commented Jan 15, 2024 •

edited

Loading

ppadti commented Jan 16, 2024 •

edited

Loading

manaswinidas commented Jan 16, 2024 •

edited

Loading

Gkrumbach07 commented Jan 16, 2024 •

edited

Loading