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

[Data Grid] Fix logic when pageSize larger than number of rows #726

Merged
merged 3 commits into from
May 2, 2023

Conversation

curq
Copy link
Contributor

@curq curq commented Apr 19, 2023

Description

There is a bug with DataGrid component, when there is less number of rows with data than number of rows per page, the DataGrid component is rendered with height equal to number of rows per page, instead of number of actual rows. This results in an empty unused space.

For example, when there is 99 rows but the number of rows is 120, the table is rendered with height that fits 120 rows, and not 99. Screenshot:
image
The issue is even more evident when we make number of rows per page even larger (e.g. 1000 per page). I explained more in the issue #699 where I provided a sandbox.

After fix:
image
You might notice that the rows per page is 99 in the screenshot (actual number of rows), but the options show 120 ( they usually are provided by the developer), so I'm not sure if this okay.

Issues Resolved

Fixes #699

Check List

  • New functionality includes testing.
  • New functionality has been documented.
  • All tests pass
    • yarn lint
    • yarn test-unit
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

For more information on following Developer Certificate of Origin and signing off your commits, please check here.

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>
@BSFishy BSFishy added the CCI College Contributor Initiative label Apr 19, 2023
@ashwin-pc
Copy link
Member

@curq I dont think you should limit the options in datagrid to fix this issue, but rather treat the row limit more like max-height like property, where if the content exceeds the size, we show only the number of rows that we limit to, but if the rows are lesser, we adjust the height to the height of the existing rows.

Look at the behavior of the list here and how this behaves as an example: https://playground.opensearch.org/app/management/opensearch-dashboards/objects
Screenshot 2023-04-19 at 10 24 50 AM

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>
Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>
@curq
Copy link
Contributor Author

curq commented Apr 20, 2023

@ashwin-pc Thank for the feedback, I have changed my fix, so that row limit behave like max-height. Now after the fix the component looks like this:

image

@ashwin-pc ashwin-pc self-assigned this Apr 26, 2023
Copy link
Member

@ashwin-pc ashwin-pc left a comment

Choose a reason for hiding this comment

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

Nice and elegant fix. Pulled this down and works as expected. I did notice that when you have say 51 rows and you select see 50 rows at a time, switching to the next page keeps the row height still at 50 even though there is just a single row. But Im pretty sure that we want it that way since I prefer avoiding a layout repaint given that the primary goal of the DataGrid is to improve rendering large volumes of data, and a layout repaint can be slow and expensive for such re-renders.

@ashwin-pc ashwin-pc assigned BSFishy and unassigned ashwin-pc Apr 27, 2023
@rednaksi91 rednaksi91 merged commit f65b5ca into opensearch-project:main May 2, 2023
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 20, 2023
* Fix logic when pageSize larger than number of rows

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>

* Remove incorrect fix

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>

* Fix logic when pageSize larger than number of rows

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>

---------

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>
(cherry picked from commit f65b5ca)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
AMoo-Miki pushed a commit that referenced this pull request Jun 20, 2023
…#825)

* Fix logic when pageSize larger than number of rows



* Remove incorrect fix



* Fix logic when pageSize larger than number of rows



---------


(cherry picked from commit f65b5ca)

Signed-off-by: Sirazh Gabdullin <sirazh.gabdullin@nu.edu.kz>
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
Labels
backport 1.x CCI College Contributor Initiative
Projects
None yet
5 participants