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

Table Component: Improve text-wrapping behavior of cells #82872

Merged

Conversation

ahuarte47
Copy link
Contributor

@ahuarte47 ahuarte47 commented Feb 16, 2024

What is this feature?

This PR fixes the visualization of strings in tables that do not fit the space of its cell. Full content is showed when user moves the cursor over the cell.

Why do we need this feature?

Long texts in cells are clipped if they do not fit the space if its cell. User can not view the full content of the string.

Who is this feature for?

[Add information on what kind of user the feature is for.]

Which issue(s) does this PR fix?:

It partially improves the issue #25623

Special notes for your reviewer:

Please check that:

  • It works as expected from a user's perspective.
  • If this is a pre-GA feature, it is behind a feature toggle.
  • The docs are updated, and if this is a notable improvement, it's added to our What's New doc.

@ahuarte47 ahuarte47 requested a review from a team as a code owner February 16, 2024 01:25
@ahuarte47 ahuarte47 requested review from nmarrs and adela-almasan and removed request for a team February 16, 2024 01:25
@grafana-pr-automation grafana-pr-automation bot added area/frontend pr/external This PR is from external contributor labels Feb 16, 2024
@ahuarte47
Copy link
Contributor Author

ahuarte47 commented Feb 16, 2024

I attach here a video before the fix, and other one with the final result applying the change.
before-fix.webm
after-fix.webm

@nmarrs nmarrs changed the title Fix text-wrapping of cells in Tables Table: Fix text-wrapping of cells Feb 16, 2024
@CptHolzschnauz
Copy link

Cool. Do you know where to find this file on a installed Grafana under Linux (Ubuntu). I would like to change it right there with nano. Feasible?

@ahuarte47
Copy link
Contributor Author

Cool. Do you know where to find this file on a installed Grafana under Linux (Ubuntu). I would like to change it right there with nano. Feasible?

In my understanding, the file that needs to be changed is a TypeScript file that needs to be built or compiled. It is not directly used by grafana like a CSS file.

@codeincarnate
Copy link
Collaborator

@ahuarte47 thanks for another contribution! I'm going to test this out quickly but generally speaking this seems like a much better way to handle this scenario!

Copy link
Collaborator

@codeincarnate codeincarnate left a comment

Choose a reason for hiding this comment

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

Just ran through this and at least for the moment I'm unable to get this to work 😢. Did a commit get missed perhaps?

@ahuarte47
Copy link
Contributor Author

ahuarte47 commented Feb 16, 2024

Sad of reading that, can I test a dashboard sample? maybe one from dev-dashboard package?
Anyway, I am attaching here the dashboard that I am showing in the videos.

My test for text-wrapping.json

NOTE:
I am testing in FireFox & Chromium (Ubuntu 22.04)

@codeincarnate
Copy link
Collaborator

Re-tested this and appears to be working fine, not sure exactly what I was doing 😕. It does get cut-off if the table is small enough, but we effectively already have that problem. We can address that as a follow-up PR. Thanks for the contribution!

@codeincarnate codeincarnate added this to the 10.4.x milestone Feb 21, 2024
@codeincarnate codeincarnate enabled auto-merge (squash) February 21, 2024 19:58
@torkelo
Copy link
Member

torkelo commented Feb 22, 2024

@ahuarte47 @codeincarnate I would not close and say this fixes #25623 , that would make a lot of people who voted for that issue upset . They are looking for text wrapping cells (not the hover effect) . But this should be a good improvement for those users (but not a complete fix)

Copy link
Member

@torkelo torkelo left a comment

Choose a reason for hiding this comment

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

Tested this PR and looks broken in Chrome Version 121.0.6167.184 (Official Build) (arm64) (OSX)

Screenshot 2024-02-22 at 08 11 53

@ahuarte47
Copy link
Contributor Author

ahuarte47 commented Feb 22, 2024

Tested this PR and looks broken in Chrome Version 121.0.6167.184 (Official Build) (arm64) (OSX)

Screenshot 2024-02-22 at 08 11 53

I am viewing everything ok as well in Chrome Version 122.0.6261.57 (Official Build) (64-bit) (Ubuntu), even with transparent background of panel enabled.

image

Maybe I should to force a background color or disable the opacity in hover?

UPDATE:
I reproduce the issue when text has not spaces;
image

I will test setting text-wrap css style.

@ahuarte47
Copy link
Contributor Author

Setting wordbreak it works better:
image

@ahuarte47 ahuarte47 force-pushed the main_#25623-fix-table-text-wrapping branch from 3bc6d31 to ef9d380 Compare February 22, 2024 22:53
@ahuarte47
Copy link
Contributor Author

ahuarte47 commented Feb 22, 2024

Please @codeincarnate @torkelo may I ask for your help to review again, thanks in advance!

@codeincarnate
Copy link
Collaborator

@ahuarte47 I'll be able to take a look this evening 😃

@codeincarnate
Copy link
Collaborator

Gave it another run-through and it appears to be working fine in each case now. Missed the issue fix label before and very much agree this doesn't close that. Thanks there @torkelo 🙏

@codeincarnate codeincarnate changed the title Table: Fix text-wrapping of cells Table Component: Improve text-wrapping behavior of cells Feb 23, 2024
@codeincarnate codeincarnate enabled auto-merge (squash) February 23, 2024 04:09
@codeincarnate codeincarnate merged commit 49299eb into grafana:main Feb 23, 2024
17 of 18 checks passed
@ahuarte47
Copy link
Contributor Author

Thanks a lot @codeincarnate !!!!

@torkelo
Copy link
Member

torkelo commented Feb 23, 2024

@ahuarte47 @codeincarnate this seems to have made it a bit worse for some scenarios, it always wraps and breaks up strings (even when there is no white space)

So large values or single strings that would just extend a bit to the right are now broken up
Screenshot 2024-02-23 at 13 23 19

Text strings that where somewhat readable in narrow columns now become very hard to read. It seems the hover view is always using the same width as the cell, the whole point of the hover effect was to give the cell more room to show the value and not be constrained by the column width.
Screenshot 2024-02-23 at 13 25 17

@ahuarte47
Copy link
Contributor Author

I can create other PR with a new option in the ui table settings to enable this. Disabled by default. Do you agree?

@torkelo
Copy link
Member

torkelo commented Feb 23, 2024

@ahuarte47 no I do want a UI option for this, can we make it so that it does not wrap until at the edge of the table?

The old behavior was so much nicer, except part of it that goes beyond the right side of the table is not visible
Screenshot 2024-02-23 at 15 09 47

@codeincarnate
Copy link
Collaborator

I can see the value of both cases, it's nice for cases with blobs of text but for longer strings it makes it more difficult to read. It may be possible to look at value length and determine from there. Agreed though that having this happen at table boundaries makes sense. I'll throw something together to see what can be done.

@codeincarnate
Copy link
Collaborator

Haven't quite finished it but testing out logic based around string length and seeing how it goes, not much at all right now as I'm mostly playing with it but progress will be in the just linked PR 😸

@ahuarte47
Copy link
Contributor Author

Haven't quite finished it but testing out logic based around string length and seeing how it goes, not much at all right now as I'm mostly playing with it but progress will be in the just linked PR 😸

I like very much this simple but effective solution using the length of the text, thank you very much!

@codeincarnate
Copy link
Collaborator

Thanks @ahuarte47 🙏

A quick update, still working through the specifics but expecting to have PR out of draft today.

yuri-tceretian pushed a commit that referenced this pull request Feb 26, 2024
* Fix text-wrapping of cells in Tables

* Set wordbreak on hover for long texts without spaces
@codeincarnate
Copy link
Collaborator

Made some updates to the mentioned PR, haven't quite gotten it working completely but I'm close and should be able to finish this one later 😄

chalapat pushed a commit to nokia/grafana that referenced this pull request Feb 27, 2024
* Fix text-wrapping of cells in Tables

* Set wordbreak on hover for long texts without spaces
@ahuarte47 ahuarte47 deleted the main_#25623-fix-table-text-wrapping branch March 4, 2024 08:18
@aangelisc aangelisc modified the milestones: 10.4.x, 10.4.0 Mar 6, 2024
@jacksongoode
Copy link

Is there a plan to allow a user to view headers that are too long in the table plugin? It seems weird that this would only be a feature for cells?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
add to changelog area/frontend area/panel/table no-backport Skip backport of PR pr/external This PR is from external contributor
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants