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 column width recalculation when vertical scrollbar hidden/shown. #12058

Merged
merged 1 commit into from Jul 23, 2020

Conversation

Ansku
Copy link
Member

@Ansku Ansku commented Jul 21, 2020

  • If the Grid has any columns with non-fixed widths, the presence of a
    vertical scrollbar affects the column width calculations. Horizontal
    scrollbar should only be shown when actually needed.

This change is Reviewable

- If the Grid has any columns with non-fixed widths, the presence of a
vertical scrollbar affects the column width calculations. Horizontal
scrollbar should only be shown when actually needed.
Copy link
Contributor

@edler-san edler-san left a comment

Choose a reason for hiding this comment

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

Reviewed 6 of 6 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @Ansku)


uitest/src/main/java/com/vaadin/tests/components/grid/GridSizeChange.java, line 29 at r1 (raw file):

        grid = new Grid<>();
        data = new ArrayList<>();
        for (int i = 0; i < 8; ++i) {

Any reason for the change of data elements?
(Maybe introduce constant for them.)


uitest/src/main/java/com/vaadin/tests/components/grid/GridSizeChange.java, line 43 at r1 (raw file):

        grid.setHeightByRows(8);
        grid.setWidth(100, Unit.PIXELS);

Same as above

Copy link
Member Author

@Ansku Ansku left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @edler-san)


uitest/src/main/java/com/vaadin/tests/components/grid/GridSizeChange.java, line 29 at r1 (raw file):

Previously, edler-san wrote…

Any reason for the change of data elements?
(Maybe introduce constant for them.)

You mean why it's 8 now when it used to be 10? That's because now it tests the scrollbar response for both scrollbars instead of just one at a time, and the tweaks are to ensure all the items within the test have the same number of digits so that we don't get a horizontal scrollbar just because row_10 takes more room than row_9 (relevant for the bit when 'Add row' is clicked). There was no particular reason to select 10 to start with, it's just an easy number to come up with.


uitest/src/main/java/com/vaadin/tests/components/grid/GridSizeChange.java, line 43 at r1 (raw file):

Previously, edler-san wrote…
        grid.setHeightByRows(8);
        grid.setWidth(100, Unit.PIXELS);

Same as above

Likewise, the initial width was selected so that it worked to test the initial setting where the column width calculations were still broken. Now that they work, it turned out that 90 pixels was just a bit too narrow for one of the new tests to work properly to test what they are supposed to test for both scrollbars at the same time. The old tests are still there and work either way.

Copy link
Contributor

@edler-san edler-san left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @edler-san)

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

3 participants