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.

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 wrap text option for pipeline run logs #2616

Merged

Conversation

DaoDaoNoCode
Copy link
Member

@DaoDaoNoCode DaoDaoNoCode commented Mar 20, 2024

JIRA: RHOAIENG-3830

Description

There is some problem with the PF log viewer, which is causing the incorrect rendering for the extremely long log lines. In the OpenShift console, the solution is to truncate the line. However, it's limited currently in the dashboard for now because we haven't applied websocket on the logs, so it's hard to truncate a single line of the logs (it's a full string now). After a discussion with the UX, we decided to add an option to allow the users to check whether they want to wrap the text in the log viewer or not. In this way, they can view the incorrectly rendered logs by unchecking the Wrap text checkbox in the toolbar.

Screenshot 2024-03-20 at 11 17 46 AM

Also, update the text in the alert because we don't truncate the exceptionally long lines :)

Screenshot 2024-03-20 at 11 18 51 AM

How Has This Been Tested?

  1. Create a pipeline run
  2. Check the logs in the steps, try to wrap and unwrap it
  3. Make sure nothing is broken
  4. Check the text in the alert

Test Impact

N/A, it's a simple update with the built-in log viewer interface.

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 or cypress tests 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

@DaoDaoNoCode
Copy link
Member Author

@yih-wang Could you check the screenshot to see if it's accurate?

@yih-wang
Copy link

@DaoDaoNoCode Can you give me a screenshot of how it looks like in the minSize of the drawer? I think it's fine to have the 'wrap text' and 'download' both outside the kebab. But I want to confirm that it doesn't cause any wrapping issues when the drawer is minSize.

@DaoDaoNoCode
Copy link
Member Author

@yih-wang Sure, this is the min size, I cannot shrink it anymore.

Screenshot 2024-03-20 at 10 36 07 PM

@yih-wang
Copy link

@DaoDaoNoCode Any mess if you start searching and input a single character in the search box?

@DaoDaoNoCode
Copy link
Member Author

@yih-wang Ah yes, it gets a little bit weird.
Screenshot 2024-03-21 at 8 02 56 AM

@yih-wang
Copy link

so that's what I meant. We need to extend the minSize of the drawer, or hide one of the 'wrap text' and 'download' into the kebab to make it neat.

@yih-wang
Copy link

That said, this kind of wrap seems acceptable to me... I'm not sure how many users would see this wrapping layout. If only 10% of them, I think we can keep it as is since keep both the wrap text and download in the toolbar would bring most convinience.

@DaoDaoNoCode
Copy link
Member Author

@yih-wang I can extend the min size a little bit. The following screenshot extends it for about 50px, do you think this is acceptable? If so I will go with this solution.
Screenshot 2024-03-21 at 8 25 27 AM

@yih-wang
Copy link

can you try to type a character in the search box? Because it will add the length of the search box.

@DaoDaoNoCode
Copy link
Member Author

@yih-wang I see, it breaks the layout again, so I added 20px more to it, then it's OK now as the screenshot shows.

Screenshot 2024-03-21 at 8 56 37 AM

@yih-wang
Copy link

I would say add at least 50px more... Since we will also have a status icon when the log is still running. And if the number (1/256) in the search box is longer, it would even take more space.

@yih-wang
Copy link

And I'm not sure if it's too large for a minSize after adding another 50px... If so, let's go back to what you originally have (with wrapped 'wrap text') since it looks not so bad.
image

@DaoDaoNoCode
Copy link
Member Author

@yih-wang How does this look like? It's 100px more compared to the very original one. If not enough I can make it to 120px more.

Screenshot 2024-03-21 at 9 01 26 AM

@DaoDaoNoCode
Copy link
Member Author

Yeah, maybe I won't change anything about the min size if you think it's not a big deal.

@yih-wang
Copy link

Sounds good to me. Let's stick to your original version.

Copy link
Contributor

@manaswinidas manaswinidas left a comment

Choose a reason for hiding this comment

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

/lgtm

Works fine.

Copy link
Member

@Gkrumbach07 Gkrumbach07 left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

Copy link
Contributor

openshift-ci bot commented Mar 21, 2024

[APPROVALNOTIFIER] This PR is APPROVED

Approval requirements bypassed by manually added approval.

This pull-request has been approved by: Gkrumbach07, jpuzz0, manaswinidas

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:

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

@openshift-merge-bot openshift-merge-bot bot merged commit 90631ea into opendatahub-io:main Mar 21, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants