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

CUMULUS-2271 Allow horizontal scrolling on table columns #897

Merged
merged 6 commits into from Dec 2, 2020

Conversation

npauzenga
Copy link
Contributor

Summary: Summary of changes

Addresses CUMULUS-2271: Dashboard: Table Column Horizontal Scroll

Changes

  • Shows the overflow on table cells when hovered

Previously the only way to show the full contents of a cell was to expand the cell's width by dragging the header. You can still do that but now, if there is overflow, you can hover on a cell and a scrollbar will appear.

Note that this removes the ellipses from truncated text. We can keep it and show the "..." when text is truncated, then remove the ellipses on hover BUT then the issue is what happens when the user moves their mouse outside of the cell? I'm not sure there's a great way to keep the full, non-truncated text in that case. We can of course handle it in JS but I'm wary of adding any handlers on a table with potentially hundreds of rows.

PR Checklist

  • Update CHANGELOG
  • Unit tests
  • Adhoc testing
  • Integration tests

@npauzenga npauzenga added the Needs Review Looking for a reviewer label Dec 1, 2020
/*&:first-of-type {
padding: 1em 0 1em 2em;
}*/
min-height: 65px;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We might not need to add a min-height here if we don't want to. I have it because without it the row will expand vertically when hovered to fit the scrollbar. It does increase all of the row heights though. If we remove it the row will expand on hover, which also might not be terrible. I could go either way.

Copy link
Member

Choose a reason for hiding this comment

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

@npauzenga I'm curious what the second option would look like with expand on hover.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jjmccoy yeah, if you comment out this line (37) and hover over the table cells, they will expand to fit the scrollbar. Happy to jump on a screenshare if that's helpful.

@jjmccoy jjmccoy self-assigned this Dec 1, 2020
@jjmccoy jjmccoy added In Review and removed Needs Review Looking for a reviewer labels Dec 1, 2020
@jjmccoy
Copy link
Member

jjmccoy commented Dec 1, 2020

This is great @npauzenga! Is there a way to show the bar on hover for some reason it only shows when I start scrolling sideways on my trackpad?

@npauzenga
Copy link
Contributor Author

npauzenga commented Dec 2, 2020

This is great @npauzenga! Is there a way to show the bar on hover for some reason it only shows when I start scrolling sideways on my trackpad?

@jjmccoy Nice catch! So we've talked a bit about this and found that it's an OS setting. I checked with Danielle this morning and she saw the same thing you were. It appears to be an issue when you use a trackpad. In your case it sounded like plugging in a mouse didn't help, in Danielle's case it did, and in my case I couldn't replicate the issue either way without changing OS settings.

The fix I just pushed forces a scrollbar to show. I was able to replicate the issue you saw by switching my OS "Show Scroll Bars" setting to "When scrolling" and this fix seems to do it. Would you mind checking again on your end?

@jjmccoy
Copy link
Member

jjmccoy commented Dec 2, 2020

This is great @npauzenga! Is there a way to show the bar on hover for some reason it only shows when I start scrolling sideways on my trackpad?

@jjmccoy Nice catch! So we've talked a bit about this and found that it's an OS setting. I checked with Danielle this morning and she saw the same thing you were. It appears to be an issue when you use a trackpad. In your case it sounded like plugging in a mouse didn't help, in Danielle's case it did, and in my case I couldn't replicate the issue either way without changing OS settings.

The fix I just pushed forces a scrollbar to show. I was able to replicate the issue you saw by switching my OS "Show Scroll Bars" setting to "When scrolling" and this fix seems to do it. Would you mind checking again on your end?

@npauzenga Thanks for investigating this. It shows on hover now. Yay!

Copy link
Member

@jjmccoy jjmccoy left a comment

Choose a reason for hiding this comment

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

LGTG!

@npauzenga npauzenga merged commit 2b20a8e into develop Dec 2, 2020
@npauzenga npauzenga deleted the feature/CUMULUS-2271-table-horizontal-scroll branch December 2, 2020 15:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants