appsmith icon indicating copy to clipboard operation
appsmith copied to clipboard

fix:pagination issues in serverside pagination listv2

Open Tooluloope opened this issue 2 years ago • 1 comments

Description

  • Fixed serverside pagination code and css
  • Disabled pagination when isLoading
  • Remove onPageSizeChange this would allow me to close #17427

Fixes #11332 Fixes #18638

Type of change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

  • Cypress

Test Plan

Add Testsmith test cases links that relate to this PR

Issues raised during DP testing

Link issues raised during DP testing for better visiblity and tracking (copy link from comments dropped on this PR)

Checklist:

Dev activity

  • [ ] My code follows the style guidelines of this project
  • [ ] I have performed a self-review of my own code
  • [ ] I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] My changes generate no new warnings
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] New and existing unit tests pass locally with my changes
  • [ ] PR is being merged under a feature flag

QA activity:

  • [ ] Test plan has been approved by relevant developers
  • [ ] Test plan has been peer reviewed by QA
  • [ ] Cypress test cases have been added and approved by either SDET or manual QA
  • [ ] Organized project review call with relevant stakeholders after Round 1/2 of QA
  • [ ] Added Test Plan Approved label after reveiwing all Cypress test

Tooluloope avatar Dec 06 '22 22:12 Tooluloope

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
appsmith ✅ Ready (Inspect) Visit Preview Dec 29, 2022 at 8:32AM (UTC)

vercel[bot] avatar Dec 06 '22 22:12 vercel[bot]

@ashit-rath, accentColor is not been used for backGround color and these wds variables were created during reskinning by @jsartisan

Tooluloope avatar Dec 14 '22 08:12 Tooluloope

@Tooluloope Observation

  • On deploying List v2 with SSP enabled, the app shows error of 'no data to display' video of List v2 : https://bthrujcsw8.vmaker.com/record/nBTi9beuyWNT2CX4

  • The issue on List v1 still remains same. Error of 'no data to display' after reaching last page Video of List v1 : https://bthrujcsw8.vmaker.com/record/oqYd1AcWsqsADoe4

QQ: This PR will also fix the current issue in List v1 as well?

chandannkumar avatar Dec 15 '22 06:12 chandannkumar

Hi @chandannkumar,

  1. The first issue occured because you enabled Inifinite Scroll. I'll be pushing a fix for that now, as we're not enabling infinite scroll in this release.

  2. The fix would only be for this List V2 not V1

Tooluloope avatar Dec 15 '22 09:12 Tooluloope

  1. The first issue occured because you enabled Inifinite Scroll.

This is also happening when Infinite Scroll is disabled

chandannkumar avatar Dec 15 '22 09:12 chandannkumar

observations:

  1. data stops rendering onPageChange during server-side pagination when Primary key is not set https://www.loom.com/share/25fea7e1f34d44bdaca13894feef6bcb
  2. Primary key gets deselected when last page is reached but when we do the JS toggle, the reference seems to be there https://www.loom.com/share/8abbe114318a4f649b92d8c55785d7a2 @Tooluloope

kamakshibhat-appsmith avatar Dec 20 '22 07:12 kamakshibhat-appsmith

@kamakshibhat-appsmith No 2. is expected, as you can see once you toggle to JS you cant toggle back. and the reason the Primary Key goes is that there's no Data at that point

@ashit-rath should we cache the keys or is that going to be too much overhead?

Tooluloope avatar Dec 20 '22 08:12 Tooluloope

@Tooluloope For number 2, I got the point why primary key goes but on the JS toggle , shouldn't the reference also be removed?

kamakshibhat-appsmith avatar Dec 20 '22 08:12 kamakshibhat-appsmith

@Tooluloope Do we have enough clarity to move this forward?

ashit-rath avatar Dec 26 '22 06:12 ashit-rath

@Tooluloope Do we have enough clarity to move this forward?

Sure, I wanted to finish up on the cypress Test first

Tooluloope avatar Dec 27 '22 10:12 Tooluloope

Tested this PR and LGTM

chandannkumar avatar Dec 29 '22 12:12 chandannkumar