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

Fix text truncation in data tables #8825

Merged
merged 1 commit into from
Jun 15, 2023

Conversation

jeffibm
Copy link
Member

@jeffibm jeffibm commented Jun 13, 2023

1. Datastores
Before
image
After
image


2. Automation Managers
Before
image
After
image


3. Service Requests
Before
image
After
image


4. Containers
Before
image
After
image


5. Container Images
Before
image
After
image


6. Instances by Providers
Before
image
After
image
Please not that icons and images are slightly smaller than before

@Fryguy
Copy link
Member

Fryguy commented Jun 13, 2023

@jrafanie @jeffibm Does this also fix #8823 ?

@Fryguy
Copy link
Member

Fryguy commented Jun 13, 2023

@jeffibm Is there a reasonable truncation limit at some point? For example, first N lines or first N bytes? - What if someone puts like 1MB of data as text?

@jrafanie
Copy link
Member

@jrafanie @jeffibm Does this also fix #8823 ?

Good question. I'm not sure. #8823 was in the pod name column shown for a specific container image, we're only seeing the list view for images and not the view for one image with several pod names. The code looks like it's treated generically so I would think it would fix it.

@Fryguy
Copy link
Member

Fryguy commented Jun 13, 2023

@jeffibm Can you verify on the pages that @jrafanie found in #8823?

@jeffibm
Copy link
Member Author

jeffibm commented Jun 14, 2023

Yea ok, will need to check how it handles very long text.
And also, how do we handle large text in PDFs,

But for now, the Longer text truncate logic is as follows -

When text.length > 300

  • The entire text will be rendered in the source, and displayed in tooltip
  • Max height of the cell becomes 200px
  • Only 15 lines will be displayed
  • An ellipses will be displayed at the end of the text to signify it has been truncated.

and it looks like this in worst case scenario..
image

Note: I have updated the screenshots in the PR's Description

@miq-bot
Copy link
Member

miq-bot commented Jun 14, 2023

Checked commit jeffbonson@d758a7d with ruby 2.6.10, rubocop 1.28.2, haml-lint 0.35.0, and yamllint
0 files checked, 0 offenses detected
Everything looks fine. 🍰

@jeffibm
Copy link
Member Author

jeffibm commented Jun 14, 2023

@jeffibm Can you verify on the pages that @jrafanie found in #8823?

I don't have data to see longer text on this particular page.
But, as Joe mentioned, the changes are generic, and they should be fixed.

@jeffibm
Copy link
Member Author

jeffibm commented Jun 15, 2023

Hey @Fryguy , @jrafanie , will these changes be ok to merge?

Copy link
Member

@jrafanie jrafanie left a comment

Choose a reason for hiding this comment

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

I don't understand all of the changes here but if we're confident this improves the truncation logic, I'm 👍 on it. My concerns about this change working optimally in various screen resolutions is something we can review after this truncation bug is fixed.

@Fryguy
Copy link
Member

Fryguy commented Jun 15, 2023

Looks good but I do have one more observation that looks strange

When a line does wrapping the vertical alignment of other cells changes to sort-of-top-aligned. However, this looks really strange especially because the vertical alignment of the header doesn't change. This is much more evident when only one line wraps but not other lines.

I hope the screenshots below show it better:

before after

@Fryguy
Copy link
Member

Fryguy commented Jun 15, 2023

That being said, I'd rather merge now because this 99% fixes a huge usability issue - let's do the rest in a follow-up

@Fryguy Fryguy merged commit e24fbf3 into ManageIQ:master Jun 15, 2023
9 checks passed
@jeffibm
Copy link
Member Author

jeffibm commented Jun 17, 2023

That being said, I'd rather merge now because this 99% fixes a huge usability issue - let's do the rest in a follow-up

Follow up PR - #8832

@Fryguy
Copy link
Member

Fryguy commented Jun 28, 2023

Backported to petrosian in commit 298effe.

commit 298effe5fc84be1a07d862b0f5464ace8f271bce
Author: Jason Frey <fryguy9@gmail.com>
Date:   Thu Jun 15 10:09:12 2023 -0400

    Merge pull request #8825 from jeffibm/fix-datatable-truncation
    
    Fix text truncation in data tables
    
    (cherry picked from commit e24fbf379772cbf0425431c60a495b642451bcd3)

Fryguy added a commit that referenced this pull request Jun 28, 2023
Fix text truncation in data tables

(cherry picked from commit e24fbf3)
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

6 participants