components
components copied to clipboard
chore: Table progressive loading (no public API)
Description
The PR introduces progressive loading feature for tables but without any public API updates yet.
The feature is added in its simplest form: the extra rows are added without col-span which is a debatable design choice and is challenging to introduce due to various design and accessibility considerations.
The PR will be merged as soon it is approved. The feature will be released upon its readiness and it will require a follow-up PR to update the public API.
Rel: sEQSANE2RASR
How has this been tested?
- New unit tests
- New integration tests
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
- Changes include appropriate documentation updates.
-
Changes are backward-compatible if not indicated, see
CONTRIBUTING.md
. -
Changes do not include unsupported browser features, see
CONTRIBUTING.md
. - Changes were manually tested for accessibility, see accessibility guidelines.
Security
-
If the code handles URLs: all URLs are validated through the
checkSafeUrl
function.
Testing
- Changes are covered with new/existing unit tests?
- Changes are covered with new/existing integration tests?
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Codecov Report
Attention: Patch coverage is 97.91667%
with 2 lines
in your changes are missing coverage. Please review.
Project coverage is 95.42%. Comparing base (
41a606f
) to head (7e24697
). Report is 3 commits behind head on main.
Files | Patch % | Lines |
---|---|---|
src/table/internal.tsx | 95.00% | 1 Missing and 1 partial :warning: |
Additional details and impacted files
@@ Coverage Diff @@
## main #2203 +/- ##
=======================================
Coverage 95.41% 95.42%
=======================================
Files 705 707 +2
Lines 18723 18788 +65
Branches 5945 5978 +33
=======================================
+ Hits 17865 17928 +63
- Misses 851 852 +1
- Partials 7 8 +1
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
The feature is added in its simplest form: the extra rows are added without col-span which is a debatable design choice and is challenging to introduce due to various design and accessibility considerations
Can you elaborate on these challenges? It seems to me like the UI has suffered quite a bit here:
The feature is added in its simplest form: the extra rows are added without col-span which is a debatable design choice and is challenging to introduce due to various design and accessibility considerations
Can you elaborate on these challenges? It seems to me like the UI has suffered quite a bit here:
The idea here is to add the loader to the first cell only instead of making the first cell span to the entire row. It has a disadvantage of not utilising the available empty space efficiently, but there are advantages as well:
- It works naturally with sticky columns;
- We can extend it by allowing custom content in other cells (e.g. we can make the loader row selectable when multi-selection) and allow populating other cells with aggregated values for those not loaded items.
Besides, that approach requires a far simpler implementation. Should we consider the col-span version, I will open a follow-up PR which will be then better straightforward to review.
Thanks for the explanations.
- It works naturally with sticky columns;
Actually even if using colspan, the loader row could stick (i.e, not be affected by horizontal scroll, and probably wrap if needed) unconditionally?
- We can extend it by allowing custom content in other cells (e.g. we can make the loader row selectable when multi-selection) and allow populating other cells with aggregated values for those not loaded items.
OK but these are hypothetical future improvements. I would like to make sure the component is in good shape now regardless of those.
Could you double-check with visual design if you haven't already to make sure this is OK?
Actually even if using colspan, the loader row could stick (i.e, not be affected by horizontal scroll, and probably wrap if needed) unconditionally?
If using colspan we can make use of all available space so it won't wrap unless the table itself is very narrow. Yes, we can make it sticky too, this actually works this way here: https://github.com/cloudscape-design/components/pull/2131
OK but these are hypothetical future improvements. I would like to make sure the component is in good shape now regardless of those.
We plan to keep the current behaviour for nested row loaders. The content of the loader might wrap or not wrap depending on the wrapLines property of the table. The loader content must not be too long - it should be designed to fit the first column or at least to not wrap for too many lines.
Could you double-check with visual design if you haven't already to make sure this is OK?
Yes, I checked already. The current plan is to keep the behaviour as is for the child rows but introduce the colSpan and make the content centered for the root loader. However, this treatment can be revisited again, that's why the current PR features the simple-most approach.