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

feat: add option to freeze columns to end #3566

Merged
merged 15 commits into from
Mar 22, 2022
Merged

Conversation

web-padawan
Copy link
Member

@web-padawan web-padawan commented Mar 15, 2022

Description

Fixes #3559

Notable features

The following features are relevant for freezing columns to end:

  • Column reordering - frozen-to-end columns can be reordered while remaining frozen to end only
  • Column resizing - frozen-to-end columns have resize handle on the left (start) to resize easier
  • Keyboard navigation - when moving horizontally to a normal cell that is visually covered by frozen-to-end cell, the grid is scrolled so that normal cell would be fully visible, same as for normal frozen columns
  • RTL support - frozen-to-end columns are positioned using JS, same as normal frozen columns

Type of change

  • Feature

@web-padawan web-padawan added the papercuts "Services Papercuts" project label Mar 15, 2022
@web-padawan web-padawan marked this pull request as ready for review March 17, 2022 09:43
@web-padawan web-padawan removed the papercuts "Services Papercuts" project label Mar 17, 2022
@DiegoCardoso
Copy link
Contributor

DiegoCardoso commented Mar 17, 2022

If I make last two columns resizable and the last one being frozen-to-end, then I can see two resize handles next to each other. Also, resizing the frozen column looks a bit broken.

(first I am resizing through the second to last column's resize handle and then trying to resize from the frozen column)

grid-resize-column.mp4

@DiegoCardoso
Copy link
Contributor

Hiding the resize handle makes it look better now.

There are two issues that I found while playing around while resizing the columns:

  • if you shrink the last column too much, the layout still getting a bit broken as you can see on the recording below:
grid-column-shrink.mp4

I tried this without setting the last column with frozenToEnd and I could somewhat reproduce the issue, so maybe we can address this issue on a separate ticket:
image

  • if you use the resize handle on the frozenToEnd column when the grid is scrolled to the end, then you cannot easily control the resizing as it happens too fast. Also on this video you can see that when you expand the column next to the frozen one, it expands beneath the frozen column, but there's no clear indication that it's working other than the scrollbar moving. The line showing that the frozen column is overlaying doesn't show immediately, only after scrolling the grid.
grid-resize-column-issue.mp4

@web-padawan
Copy link
Member Author

Also on this video you can see that when you expand the column next to the frozen one, it expands beneath the frozen column, but there's no clear indication that it's working

We can try scrolling the grid horizontally in this case so that resize handle would be always visible.
But I'm not sure if this is a good idea. Checked Google Sheets and they do not work this way:

scroll.mp4

So it looks like resizing a column past the viewport is not technically supported there.
Instead, you can only resize as far as the available space on the screen.

BTW there is no support for columns frozen to the end in Sheets 🙂

@DiegoCardoso
Copy link
Contributor

One thing we can try to do is to show the vertical line when resizing to the right makes the frozenToEnd column overlay the other column. Basically, this kind of resize makes scroll position move from the end of the horizontal scrollbar, so the vertical border would naturally appear on this case.

@web-padawan
Copy link
Member Author

I've tried to implement this behavior, here is the result. Would this be good enough in your opinion?

scroll-resize.mp4

:

@DiegoCardoso
Copy link
Contributor

I think it's better this way.

@web-padawan
Copy link
Member Author

Added logic and test to implement the behavior described in #3566 (comment)

@DiegoCardoso
Copy link
Contributor

Tested it and the visual indicator is indeed better.

Found two issue with resizing the column:

  • First, after you resize the column before the frozen-to-end to expand, then when you click on the horizontal scrollbar, the grid returns to the first column (you can't see but I am clicking on the scrollbar thumb):
grid-scroll-issue.mp4
  • I can still see the issue when the last column is shrunk past a certain point, when trying to expand it again makes the column look broken. It looks that the issue happens when we shrink the column using its own resize handle, but when we want to expand it back, we are using the handle from its sibling. And the column doesn't expand to take the available space left.
grid-column-shrink-bug.mp4

@web-padawan
Copy link
Member Author

when you click on the horizontal scrollbar, the grid returns to the first column

Confirmed. I could reproduce it without frozenToEnd column also, see #3586

@sonarcloud
Copy link

sonarcloud bot commented Mar 21, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 5 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@web-padawan
Copy link
Member Author

when the last column is shrunk past a certain point, when trying to expand it again makes the column look broken. It looks that the issue happens when we shrink the column using its own resize handle, but when we want to expand it back, we are using the handle from its sibling

Confirmed. This is partially caused by the fact that minimum header cell width is set to 10 here:

But the actual width of the resize handle is at least 18px (when frozen column is the last one):

[last-column] [part~='resize-handle']::before,
[last-frozen] [part~='resize-handle']::before {
width: 18px;

This magic number has been there since the original implementation: vaadin/vaadin-grid@fbed07c

I'm not sure if that's a good idea to use such a small "magic number" - it allows you to shrink the column so that it practically disappears. Maybe we could increase this hardcoded value to 35 so that column remains at least partially visible.

Copy link
Contributor

@DiegoCardoso DiegoCardoso left a comment

Choose a reason for hiding this comment

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

Since we have tickets describing the issues raised on this PR, we can proceed with it and take care of them on separate PRs.

@vaadin-bot
Copy link
Collaborator

This ticket/PR has been released with Vaadin 23.1.0.alpha1 and is also targeting the upcoming stable 23.1.0 version.

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.

[grid] Add web component logic to freeze column to end
5 participants