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

Optimize scrolling behavior of Discover table #6683

Merged
merged 1 commit into from
May 1, 2024

Conversation

AMoo-Miki
Copy link
Collaborator

Description

This change addresses three problems by lazy-loading additional rows when infinitely scrolling the table:

  1. While a page size of 10 helped speed up rendering, scrolling users encountered the bottom of the table too often. By continuing to load a page but lazy-loading 4 additional pages (for a total of 5), that frequency is reduced.
  2. As new rows were mounted, their content pushed cell boundaries and caused column widths to change; this was unpleasant and distracting. After the first page is loaded, the columns are allowed to adjust to the content and then the dimensions are fixed to prevent that.
  3. Allow the paginated experience have the 50 pages that were previously allocated to it.

Issues Resolved

TBA

Screenshot

Before

Before.mov

After

After.mov

Changelog

  • skip

Check List

  • All tests pass
    • yarn test:jest
    • yarn test:jest_integration
  • New functionality includes testing.
  • New functionality has been documented.
  • Update CHANGELOG.md
  • Commits are signed per the DCO using --signoff

@@ -39,7 +39,7 @@ export function TableHeader({
}: Props) {
return (
<tr data-test-subj="docTableHeader" className="osdDocTableHeader">
<th style={{ width: '24px' }} />
<th style={{ width: '28px' }} />
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

24px for the button + 4px for a right padding = 28px

Copy link

codecov bot commented Apr 30, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.66%. Comparing base (d2d410b) to head (0e909d6).
Report is 49 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff             @@
##             main    #6683       +/-   ##
===========================================
+ Coverage   32.93%   67.66%   +34.72%     
===========================================
  Files        2260     3415     +1155     
  Lines       45769    66886    +21117     
  Branches     7200    10884     +3684     
===========================================
+ Hits        15075    45259    +30184     
+ Misses      29984    19025    -10959     
- Partials      710     2602     +1892     
Flag Coverage Δ
Linux_1 33.18% <ø> (+0.24%) ⬆️
Linux_3 45.23% <ø> (?)
Linux_4 34.85% <ø> (?)
Windows_1 33.20% <ø> (?)
Windows_2 55.56% <ø> (?)
Windows_3 45.23% <ø> (?)
Windows_4 34.85% <ø> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AMoo-Miki AMoo-Miki force-pushed the disover-table-layout branch from dbb8be3 to a27f2c9 Compare April 30, 2024 00:46
@AMoo-Miki AMoo-Miki added backport 2.x v2.14.0 and removed Skip-Changelog PRs that are too trivial to warrant a changelog or release notes entry labels Apr 30, 2024
@kavilla
Copy link
Member

kavilla commented Apr 30, 2024

@AMoo-Miki for the test failure, I think it might be worth to reduce the number instead of having the test build in the non-optimized version. Versus modifying your logic for the test to pass.

Also:
* some minor cleanup

Signed-off-by: Miki <[email protected]>
@AMoo-Miki AMoo-Miki force-pushed the disover-table-layout branch from a27f2c9 to 0e909d6 Compare April 30, 2024 18:10
@AMoo-Miki
Copy link
Collaborator Author

@AMoo-Miki for the test failure, I think it might be worth to reduce the number instead of having the test build in the non-optimized version. Versus modifying your logic for the test to pass.

Since retry.try has a huge timeout of 2 mins, it is safe to assume that the lazy-loading would have run in the given time to render the 50 rows that we would expect.

@kavilla
Copy link
Member

kavilla commented Apr 30, 2024

@AMoo-Miki for the test failure, I think it might be worth to reduce the number instead of having the test build in the non-optimized version. Versus modifying your logic for the test to pass.

Since retry.try has a huge timeout of 2 mins, it is safe to assume that the lazy-loading would have run in the given time to render the 50 rows that we would expect.

oh so you are saying it's an actual failure? i thought maybe the test might have just been actually expecting 50 rows that aren't in fully view but since we optimized it wasn't able to find those rows unless it was scrolled with

Copy link
Member

@kavilla kavilla left a comment

Choose a reason for hiding this comment

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

one test is flaky. and the link checker has been fixed. lgtm!!

@AMoo-Miki AMoo-Miki merged commit 9ac5203 into opensearch-project:main May 1, 2024
67 of 69 checks passed
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 1, 2024
Also:
* some minor cleanup

Signed-off-by: Miki <[email protected]>
(cherry picked from commit 9ac5203)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 1, 2024
Also:
* some minor cleanup

Signed-off-by: Miki <[email protected]>
(cherry picked from commit 9ac5203)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kavilla pushed a commit that referenced this pull request May 1, 2024
Also:
* some minor cleanup


(cherry picked from commit 9ac5203)

Signed-off-by: Miki <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
kavilla pushed a commit that referenced this pull request May 1, 2024
Also:
* some minor cleanup


(cherry picked from commit 9ac5203)

Signed-off-by: Miki <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
LDrago27 pushed a commit to LDrago27/OpenSearch-Dashboards that referenced this pull request Jun 3, 2024
AMoo-Miki added a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Jun 13, 2024
AMoo-Miki added a commit to AMoo-Miki/OpenSearch-Dashboards that referenced this pull request Jun 13, 2024
AMoo-Miki added a commit that referenced this pull request Jun 13, 2024
* Optimize scrolling behavior of Discover table

This is an improvement to #6683

Signed-off-by: Miki <[email protected]>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 13, 2024
* Optimize scrolling behavior of Discover table

This is an improvement to #6683

Signed-off-by: Miki <[email protected]>
(cherry picked from commit ebe2b9f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 13, 2024
* Optimize scrolling behavior of Discover table

This is an improvement to #6683

Signed-off-by: Miki <[email protected]>
(cherry picked from commit ebe2b9f)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AMoo-Miki pushed a commit that referenced this pull request Jun 13, 2024
* Optimize scrolling behavior of Discover table

This is an improvement to #6683


(cherry picked from commit ebe2b9f)

Signed-off-by: Miki <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AMoo-Miki pushed a commit that referenced this pull request Jun 13, 2024
* Optimize scrolling behavior of Discover table

This is an improvement to #6683


(cherry picked from commit ebe2b9f)

Signed-off-by: Miki <[email protected]>
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
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.

3 participants