components icon indicating copy to clipboard operation
components copied to clipboard

chore: Table progressive loading (no public API)

Open pan-kot opened this issue 9 months ago • 5 comments

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

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.

pan-kot avatar Apr 26 '24 14:04 pan-kot

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.

codecov[bot] avatar Apr 26 '24 15:04 codecov[bot]

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:

Screenshot 2024-04-29 at 15 00 28

jperals avatar Apr 29 '24 13:04 jperals

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:

Screenshot 2024-04-29 at 15 00 28

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:

  1. It works naturally with sticky columns;
  2. 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.

pan-kot avatar Apr 29 '24 13:04 pan-kot

Thanks for the explanations.

  1. 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?

  1. 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?

jperals avatar Apr 29 '24 13:04 jperals

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.

pan-kot avatar Apr 30 '24 08:04 pan-kot