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

Refactor/table component #2838

Open
wants to merge 5 commits into
base: feat/table-component
Choose a base branch
from

Conversation

chrismclarke
Copy link
Member

@chrismclarke chrismclarke commented Mar 10, 2025

PR Checklist

  • PR title descriptive (can be used in release notes)

Breaking Changes

Proposes replacing show_id authoring parameter with a column_list parameter that can optionally name columns to show (order preserved)

Hardcoded sizing (80vh max-height) is proposed to be removed in favour of defining inline authoring styles and allowing table to default scroll within full page

Description

Targets branch of #2786

  • Add virtualised scroller to improve rendering performance for large data lists
  • Add data searchbox and filter functionality
  • Convert to standalone component to improve import handling and opt-out optimisation
  • Remove show_id authoring parameter and replace with a more general column_list parameter that authors can use to define the names and order of columns to display
  • Update component_table examples to include a mix of basic, advanced and large-data demos
  • Fix async computed signal issue and convert table data source to signal
  • Tidy up author/component parameter mapping

Review Notes

See updated demo. Note breaking changes that may need update for deployments currently targeting the same branch as this PR

Dev Notes

To solve the issue of poor rendering performance on large lists I initially tried to implement a mat-table paginator (as you had also mentioned in original PR), which did function well to limit the number of rendered results. There were however a few shortcomings with this approach, notably:

  1. It required import of various material core and theming CSS to display generated UI in any reasonable way. This would require further work to integrate into theming in general and get particularly confusing for our different app themes. It would also require significant refactor when migrating from angular 17 -> 19 due to various theme breaking changes between versions
  2. It included hardcoded text that would be tricky to incorporate into translation system
  3. It would require exposing pagination options to authoring which could get confusing

So instead I opted for including a virtualised scroller instead, so that all rows can be displayed but only those visible in the viewport rendered. Virtual scrollers are often fiddly, usually relying on fixed item heights to calculate scroll bounds. I opted to use an experimental scroller from angular cdk which has support for dynamic item heights. It appears to work well for the current demo case, so I think I feel reasonably comfortable making this the default behaviour for now, and could possibly investigate author-driven opt-out or alternate pagination if there is good reason to do so in the future.

As for whether angular-material is good to include in the repo or not, I'm probably leaning towards a no given the amount of effort it took when trying to use any of the functionality that is dependent on theming (and so prone to break in the future); it also gives another component library to keep updated with angular (when we already struggle to keep ionic updated). That being said, I think the table component is working quite nice as-is for now so would be inclined to keep until there is strong need for more table functionality (at which point I'd probably recommend refactoring to use the more primative cdk/table and recreating UI features we need such as sort icons, or start fully from scratch with our own UI base on headless tanstack-table). I would say more generally best to avoid angular-material components where possible moving forwards, but open to using tools from the angular-cdk packages which typically underpin most of the components.

If we were to go down the route of custom table component based on tanstack, should note there is also a tanstack-virtual library which can be used with tables, e.g. https://tanstack.com/virtual/v3/docs/framework/angular/examples/table

I haven't tested whether making the component standalone will correctly opt it out during optimisation processes or not, could be worth a quick test post merge on one of the deployment repos that is not using table component.

Follow-up

  • Add support for rendering live data (either adding subscription in this component or via a separate data_table component
  • Add support for row-click authoring and event binding (with row data item context available to action)
  • Considering adding support for begin_table/end_table child rows that could be used to provide specific templating for named columns (e.g. rendering an image in a column that includes the image path)

Git Issues

Closes #

Screenshots/Videos

Example - updated component data demo

Note - default doesn't actually sort by id like demo text says, default is just row order

Note - the large table scrolling appears to stop/glitch during scrolling but this is actually manual stop to view data. The scroll and render remains fully responsive throughout

Screenity.video.-.Mar.9.2025.1.mp4

@chrismclarke chrismclarke requested a review from jfmcquade March 10, 2025 05:02
@chrismclarke chrismclarke mentioned this pull request Mar 10, 2025
3 tasks
@chrismclarke chrismclarke changed the title Review/table component Refactor/table component Mar 10, 2025
@chrismclarke chrismclarke marked this pull request as ready for review March 10, 2025 20:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant