Improves the DataTable RowStatus [FC-0036]

[pomegranited]

Description

Modifies the DataTable RowStatus to read, e.g. "Showing 1 - 10 of 500" instead of just "Showing 10 of 500".

Closes openedx/modular-learning#111

Private-ref: FAL-3534

Deploy Preview

https://deploy-preview-2838--paragon-openedx.netlify.app/components/datatable/#frontend-filtering-and-sorting

Testing instructions

1. Visit the deploy preview for the DataTable component
2. Navigate the table using the various pagination controls
3. Ensure that the modified "Showing ..." text is accurate and updates as the page changes.

Merge Checklist

• If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
• Does your change adhere to the documented style conventions?
• Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
• Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
• Were your changes tested in the example app?
• Is there adequate test coverage for your changes?
• Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

• Verify your changes were released to NPM at the expected version.
• If you'd like, share your contribution in #show-and-tell.
• 🎉 🙌 Celebrate! Thanks for your contribution.

[openedx-webhooks]

Thanks for the pull request, @pomegranited! Please note that it may take us up to several weeks or months to complete a review and merge your PR.

Feel free to add as much of the following information to the ticket as you can:

• supporting documentation
• Open edX discussion forum threads
• timeline information ("this must be merged by XX date", and why that is)
• partner information ("this is a course on edx.org")
• any other information that can help Product understand the context for the PR

All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here.

Please let us know once your PR is ready for our review and all tests are green.

[netlify]

✅ Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name	Link
🔨 Latest commit	67488a6
🔍 Latest deploy log	https://app.netlify.com/sites/paragon-openedx/deploys/655d789dd063ee0008012be9
😎 Deploy Preview	https://deploy-preview-2838--paragon-openedx.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

---

To edit notification comments on pull requests, go to your Netlify site configuration.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (7f222d1) 92.83% compared to head (67488a6) 92.83%.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #2838   +/-   ##
=======================================
  Coverage   92.83%   92.83%           
=======================================
  Files         235      235           
  Lines        4240     4244    +4     
  Branches     1029     1031    +2     
=======================================
+ Hits         3936     3940    +4     
  Misses        300      300           
  Partials        4        4           

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

[ali-hugo]

@pomegranited Everything seems to be working perfectly for me. Thanks!

[ChrisChV]

@pomegranited The indexes have strange changes in the "Backend filtering and sorting" table. (watch the video). It seems to update early and the second update looks strange

https://www.loom.com/share/72a23575fd264bb48de59ec2838a0a33?sid=c4d7258b-6c8c-4601-acc4-8b65de020fff

[pomegranited]

@ChrisChV

The indexes have strange changes in the "Backend filtering and sorting" table. (watch the video). It seems to update early and the second update looks strange

Thanks for noticing this.. but I'm not sure what to do about it. I think the progressive updates are because of the async backend queries, but since they eventually resolve to being the right numbers, that might be enough? I'm happy to ask upstream about how we can handle this better.

[ChrisChV]

Thanks for noticing this.. but I'm not sure what to do about it. I think the progressive updates are because of the async backend queries, but since they eventually resolve to being the right numbers, that might be enough? I'm happy to ask upstream about how we can handle this better.

@pomegranited I understand, I think it is a good idea to ask upstream, also so that they have this behavior registered

[ChrisChV]

@pomegranited 👍

• I tested this: I tested the change on the sanbox with all the tables
• I read through the code and considered the security, stability and performance implications of the changes.
• I tested that the UI can be used with a keyboard only (tab order, keyboard controls).
• Includes tests for bugfixes and/or features added.
• Includes documentation
• [n/a] Includes fixtures that create objects required for manual testing.

[bradenmacdonald]

@pomegranited @ChrisChV

The problem is this line:

paragon/src/DataTable/RowStatus.jsx

Line 10 in 67488a6

const rowCount = page?.length || rows?.length;

What I think is happening: While the requested page is loading, the table still holds the previous value of page and so page.length is incorrect. (Or maybe it's rows - I'm not sure which is which, but the effect is the same.) This results in rowCount being sometimes wrong while the data is loading, which causes lastRow to be wrong as well. (firstRow is fine since it doesn't depend on the invalid rowCount).

This is a symptom of a bigger problem: while the next/previous page is loading, the table doesn't display any indicator that it's loading, and still uses values from whatever page was previously displayed. IMHO the best fix is that as soon as the user clicks onto another page, the variable that holds the data (page or rows) should immediately update to hold placeholder data kind of like this:

And (importantly) that placeholder data should have the correct numbers of rows. If there are 7 items total and you're on the third page with a page size of three, the placeholder should only have 1 row, not 3. Then (with no changes to any of your code), the page counts will be accurate the whole time. Plus the user will get important visual feedback when the table is valid (loaded) or not.

So my suggestion would be to merge this code as is, since it doesn't make anything worse, and discuss with the paragon maintainers then open a ticket to add a proper loading state to the DataTable component.

Here is a screenshot showing the bug in the current master version of paragon - the pagination says this is page 3, but the data is from page 2 while page 3 loads. And the number on the left is wrong.

[pomegranited]

Thanks for your assessment @bradenmacdonald . I had a quick play to see if I could handle data loading better here, but since the bulk of the functionality is in react-table, I'm not seeing clearly what needs to be changed. Will see what upstream has to say..

[pomegranited]

@bradenmacdonald Who should I ping for upstream review here?

[bradenmacdonald]

Who should I ping for upstream review here?

@arbrandes We've made this improvement to Paragon as part of the Tagging/Taxonomy Axim funded contribution project, but I'm not sure how to get it reviewed. Would you mind giving us a nudge in the right direction?

[arbrandes]

For this one I'll defer to @brian-smith-tcril.

[brian-smith-tcril]

This looks great! I'll bring it up in the working group meeting tomorrow morning to make sure everyone is happy with the changes.

[brian-smith-tcril]

@bradenmacdonald I will also bring up adding a loading indicator as you mentioned here in the WG meeting tomorrow.

[bradenmacdonald]

Awesome, thanks @brian-smith-tcril !

[bradenmacdonald]

@brian-smith-tcril Would you mind merging this for us, if it's good to go? I don't think we have permission.

[openedx-webhooks]

@pomegranited 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future.

[openedx-semantic-release-bot]

🎉 This PR is included in version 21.8.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[pomegranited]

Thank you for merging this change @brian-smith-tcril !
Do I need to submit version update PRs for the MFEs we want to use this with? Or does that happen automatically somehow?

CC @arbrandes

[openedx-semantic-release-bot]

🎉 This PR is included in version 22.0.0-alpha.19 🎉

The release is available on:

• npm package (@alpha dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[arbrandes]

@pomegranited, while Renovate will issue upgrade PRs for packages in the @edx namespace, it is not yet consistent across the board. If you're looking to see this in a particular MFE, the best thing to do would be to issue a PR that upgrades it manually.