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 performance issue in Grid initial render with Footer #10705

Merged
merged 1 commit into from
Mar 13, 2018

Conversation

tsuoanttila
Copy link
Contributor

@tsuoanttila tsuoanttila commented Mar 12, 2018

This change is Reviewable

@TatuLund
Copy link
Contributor

:lgtm:


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@elmot
Copy link
Contributor

elmot commented Mar 13, 2018

Review status: all files reviewed at latest revision, 1 unresolved discussion, all commit checks successful.


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

        }
        grid.setItems(IntStream.range(0, 10).boxed().map(i -> ""));
        grid.appendFooterRow();

How does it work if there is no footer rows?
How does it work if there are more than one footer rows?

Looks like this line should have a comment.


Comments from Reviewable

@tsuoanttila
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 1 unresolved discussion.


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

Previously, elmot (Ilia Motornyi) wrote…

How does it work if there is no footer rows?
How does it work if there are more than one footer rows?

Looks like this line should have a comment.

This test UI is used to render and report the rendering times for TeamCity tracking. The Footer is not used for anything, the UI itself is not used for anything other than render and report spent time.

I don't personally really care wether the footer is there or not, I'm not sure if these statistics are being gathered from the validation builds (most likely should be). It should be there as it displayed a pretty huge factor in this case (increasing the rendering time by about 18 seconds), but not really mandatory.

Should I remove it or keep it? How should I comment it?


Comments from Reviewable

@elmot
Copy link
Contributor

elmot commented Mar 13, 2018

:lgtm:


Review status: :shipit: all files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@elmot elmot merged commit 5deec67 into master Mar 13, 2018
@elmot elmot deleted the grid_footer_performance branch March 13, 2018 11:55
tsuoanttila added a commit that referenced this pull request Mar 20, 2018
* Fix performance issue in Grid initial render with Footer
@tsuoanttila tsuoanttila added this to the 8.3.3 milestone Mar 20, 2018
tsuoanttila added a commit that referenced this pull request Mar 27, 2018
* Fix performance issue in Grid initial render with Footer
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.

3 participants