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

Create VirtualizedGrid component for catalog view #5795

Merged
merged 7 commits into from Jul 10, 2020

Conversation

sahil143
Copy link
Contributor

@sahil143 sahil143 commented Jun 22, 2020

Fixes: https://issues.redhat.com/browse/ODC-4027

Analysis: Virtualize Developer Catalog to support very large number of items

Solution: Created a VirtualizedGrid component which is used in catalog
NOTE: this also affects the operator hub because same component is shared between the catalog page and operator hub

Screenshots:

GroupBy-min

Unit tests:
Screenshot from 2020-06-22 22-16-48
Screenshot from 2020-06-23 13-33-45

Browser:

  • Chrome

@openshift-ci-robot openshift-ci-robot added the component/core Related to console core functionality label Jun 22, 2020
@openshift-ci-robot openshift-ci-robot added the component/shared Related to console-shared label Jun 22, 2020
@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2020
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jun 23, 2020
@sahil143 sahil143 force-pushed the virtualize-catalog branch 2 times, most recently from 239fbc5 to e4512b9 Compare June 23, 2020 16:42
@rohitkrai03
Copy link
Contributor

/assign

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

@sahil143 I haven't gone into reviewing code in depth yet, but noticed a few issues by just running your branch locally. To start off with the performance really seems to be slow and the frame rates are dropping too much with the new implementation. While scrolling the screen feels janky due to very low frame rates.

Below is a gif of dev catalog on your branch, notice the low frame rate shown in the top left corner. Its almost always below 10fps.
Peek 2020-06-24 02-12

Below is a dev catalog on master, notice how the frame rate goes up to 60fps.

Peek 2020-06-24 02-19

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

I also see a lot of react warnings in the console.

Screenshot from 2020-06-24 02-13-45

Screenshot from 2020-06-24 02-14-10

Screenshot from 2020-06-24 02-14-16

3-45.png…]()

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

I see the bottom of the tiles being cut off when there are less number of items in catalog -
Screenshot from 2020-06-24 02-02-26

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

Also noticed this weird issue where the tiles suddenly vanish from the top while scrolling up and down and then come back suddenly.
Screenshot from 2020-06-24 02-03-40

Peek 2020-06-24 02-14

@sahil143
Copy link
Contributor Author

I also see a lot of react warnings in the console.

fixed, except for the key prop missing. Key Prop is provided on both CellMeasurer and wrapper div but somehow it's being removed in react-virtualized.

I see the bottom of the tiles being cut off when there are less number of items in catalog -

height of the container is being set by the window scroller and the tiles are not cut it's just that their box shadow is hidden. i tried adding margin which adds the space but the shadow is still hidden for the last row of tiles.

Also noticed this weird issue where the tiles suddenly vanish from the top while scrolling up and down and then come back suddenly.

I wasn't able to reproduce this while scrolling. Is there any particular step that needs to be followed here?

To start off with the performance really seems to be slow and the frame rates are dropping too much with the new implementation. While scrolling the screen feels janky due to very low frame rates.

yes, the frame rate seems to be very low, Investigating this issue.

@sahil143
Copy link
Contributor Author

/retest

@christianvogt
Copy link
Contributor

Scrolling is a bit janky it seems because the scrollbar grows and shrinks in size as your scroll down and up. I would expect the scrollbar to remain relatively the same size. After scrolling through the entire list, I would expect the cache to have a precise measurement on the scrollbar and to no longer adjust the sizing. But that's not the case here.

@christianvogt
Copy link
Contributor

christianvogt commented Jun 25, 2020

The tiles rendered to screen seem to be re-rendering on every scroll. I'd expect them to not have to re-render unless data changes.

edit: the items are actually being unmounted and remounted on scroll

@christianvogt
Copy link
Contributor

The last row in the grid disappears prematurely when scrolling up.

@sahil143
Copy link
Contributor Author

/hold

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 29, 2020
@sahil143
Copy link
Contributor Author

sahil143 commented Jul 6, 2020

Scrolling is a bit janky it seems because the scrollbar grows and shrinks in size as your scroll down and up. I would expect the scrollbar to remain relatively the same size. After scrolling through the entire list, I would expect the cache to have a precise measurement on the scrollbar and to no longer adjust the sizing. But that's not the case here.

the scrollbar is growing and shrinking because of dynamic row/cell height, I tried making the row height fixed and the scrollbar remained the same size.

The tiles rendered to screen seem to be re-rendering on every scroll. I'd expect them to not have to re-render unless data changes.
edit: the items are actually being unmounted and remounted on scroll

I guess that's more or less expected, this issue talks about the same bvaughn/react-virtualized#416

The last row in the grid disappears prematurely when scrolling up

fixed.

@sahil143
Copy link
Contributor Author

sahil143 commented Jul 6, 2020

/hold cancel

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 6, 2020
@sahil143
Copy link
Contributor Author

sahil143 commented Jul 6, 2020

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 6, 2020
@christianvogt
Copy link
Contributor

christianvogt commented Jul 6, 2020

Scrolling is a bit janky it seems because the scrollbar grows and shrinks in size as your scroll down and up. I would expect the scrollbar to remain relatively the same size. After scrolling through the entire list, I would expect the cache to have a precise measurement on the scrollbar and to no longer adjust the sizing. But that's not the case here.

the scrollbar is growing and shrinking because of dynamic row/cell height, I tried making the row height fixed and the scrollbar remained the same size.

Shouldn't we be able to assume a certain height per row?
The difference in scrollbar size on page load and after scrolling is quite noticeable.

On page load:
image

After scrolling to the bottom:
image

@christianvogt
Copy link
Contributor

The tiles rendered to screen seem to be re-rendering on every scroll. I'd expect them to not have to re-render unless data changes.
edit: the items are actually being unmounted and remounted on scroll

I guess that's more or less expected, this issue talks about the same bvaughn/react-virtualized#416

This is not expected. Components should be unmounted and remounted on scroll.
Furthermore, tiles can be wrapped with memo to ensure we don't need to re-render.

@christianvogt
Copy link
Contributor

Missing padding at the bottom. Also it seems that the background is now focusable which causes a focus outline (see black outline). Previously it was not.

image

Initial implementation

add scss and resolve styling issues

finish implementation

cleanup
fix imports and add comments

add unit tests for cell
css and test

fix utils tests
remove cache from cell tests
@openshift-ci-robot openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 7, 2020
@sahil143
Copy link
Contributor Author

sahil143 commented Jul 7, 2020

Shouldn't we be able to assume a certain height per row?
The difference in scrollbar size on page load and after scrolling is quite noticeable.

yes, I missed a prop estimatedRowHeight. After adding this prop scroll bar size is consistent.

This is not expected. Components should be unmounted and remounted on scroll.
Furthermore, tiles can be wrapped with memo to ensure we don't need to re-render.

As discussed, Added React.memo to the renderTile component to prevent unnecessary re-renders.

Missing padding at the bottom.

fixed this. used padding for each cell instead of subtracting a constant from height and width to create space between tiles

Updated Screenshots
Screenshot 2020-07-08 at 12 05 32 AM
Screenshot 2020-07-08 at 12 05 56 AM

… to use padding instead of subtracting the height width to create spacing

fix unit tests
Copy link
Contributor

@christianvogt christianvogt left a comment

Choose a reason for hiding this comment

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

I see this console:
image
image

When using the type filters, even when there's enough space for more columns, sometimes it shows less:
image

@sahil143
Copy link
Contributor Author

sahil143 commented Jul 9, 2020

I see this console:
image
image

When using the type filters, even when there's enough space for more columns, sometimes it shows less:
image

@christianvogt fixed both the issue.

updated screenshot
Screenshot 2020-07-09 at 6 18 55 PM

@christianvogt
Copy link
Contributor

/lgtm
/approve

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: christianvogt, sahil143

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 9, 2020
@rohitkrai03
Copy link
Contributor

/retest

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@rohitkrai03
Copy link
Contributor

@sahil143 It would also be good if you could squash your commits into better explanatory commit messages around the feature.

@openshift-merge-robot openshift-merge-robot merged commit de042e5 into openshift:master Jul 10, 2020
@spadgett spadgett added this to the v4.6 milestone Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/core Related to console core functionality component/shared Related to console-shared lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants