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

faster sortable table #1740

Merged
merged 31 commits into from
Mar 25, 2025
Merged

faster sortable table #1740

merged 31 commits into from
Mar 25, 2025

Conversation

michaelglass
Copy link
Contributor

@michaelglass michaelglass commented Mar 19, 2025

What changes does this release include?

Adds a more performant sortable table by caching sorted data in the state.

easiest to review commit-by-commit

How has the API changed?

Please paste the output of elm diff run on latest master in the code block:

This is a MINOR change.

---- ADDED MODULES - MINOR ----

    Nri.Ui.SortableTable.V6

Rejiggers the API for SortableTable a bit.

  • State becomes opaque, and is renamed Model
  • because of this, encoders and decoders are included for the Model
  • because of caching & opaque model, implements TEA and exposes an update function
  • in order to initialize the model, dev now needs to pass in all columns
  • when the SortableTable has no model attribute, it is "Loading", else it is loaded.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@bcardiff
Copy link
Member

@bcardiff do you want me to support pagination here?

Only if you find a nice way to

  • make it optional
  • don't make it dependent of a specific pagination control

Another thing I recall is that SortableTable is for client only data, but it has some styling on headers. Is there a way to have a config that would make SortableTable work with a remote data source also? I think that supporting optional pagination and remote data source will allow us to design a better component api.

But, we can always iterate on a V7 with that and leave pagination and remote out of the picture for now.

@michaelglass michaelglass force-pushed the faster-sortable-table branch 2 times, most recently from 6e7043c to cb83dc4 Compare March 19, 2025 23:59
@@ -326,15 +314,15 @@ dataWithCode =

{-| -}
type alias State =
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💭 let's make this type opaque

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes!

@michaelglass michaelglass force-pushed the faster-sortable-table branch from f8b3ddd to 42525da Compare March 20, 2025 11:49
@michaelglass michaelglass force-pushed the faster-sortable-table branch from 870e3bf to f28e4e2 Compare March 20, 2025 14:21
@michaelglass michaelglass marked this pull request as ready for review March 20, 2025 14:23
@michaelglass michaelglass force-pushed the faster-sortable-table branch from 07de989 to 0800441 Compare March 24, 2025 14:50
@michaelglass michaelglass force-pushed the faster-sortable-table branch from 74bdc4c to 2ef31c5 Compare March 24, 2025 15:14
@michaelglass michaelglass enabled auto-merge March 24, 2025 15:21
@michaelglass michaelglass requested a review from bcardiff March 24, 2025 15:21
@michaelglass michaelglass force-pushed the faster-sortable-table branch from c2655d4 to 678adf7 Compare March 24, 2025 15:24
{ column : id
, sortDirection : SortDirection
, entries : Dict.Dict ( id, SortDirection ) (List entry)
, sorter : id -> Sorter entry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see we need this to rebuild the model with new entries. The config of the table is usually in the view function.

This is fine, I personally would move into an idiom of having a tableConfig as a top level function that we can use from the view and from the update. That way we avoid the need to store functions in the model. But is fine as is. Just sharing thoughts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you elaborate here. I don't totally see what you'd do to make this better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we do

type alias Columns id entry msg = List (Column id entry msg)

rebuild : Config data id -> Model id entry -> List entry -> Model id entry
view : Config data id -> List (Attribute id entry msg) -> Html msg -- flip the order of the first two arguments
update : Config data id -> Msg id -> Model id entry -> Model id entry

Then the model no longer needs the sorter. It deviates from other Nri.Ui components a little bit. But the config is usally fixed and bound to a specific "table instance", but we need to wire things manually for each rebuil / view / update.

Otherwise we could give more "essence" to a "table instance"

myTable = 
  let
    config = ...
  in
  { rebulid = SortableTable.rebuild config
  , view = SortableTable.view config
  , update = SortableTable.update config
  }

view = ...
  myTable.view ...

Again, nothing wrong with the current design. Just a thought.



{-| -}
init : id -> List (Column id entry msg) -> List entry -> Model id entry
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the init, rebuild haves a mandatory List entry and the config can have a missing state it seems the user will be forced to do some tricky stuff to implement a "refresh button".

Should we make init and rebuild have a Maybe (List entry) instead?

I imagine:

  • the app makes a request to load some entries remotely
  • we don't have a table model yet.. ok. In the view I will not pass a model doing List.filerMap or something
  • the app get the request
  • we can initialize a table model now, with some choice of which column to sort with
  • the user changes the sorting
  • we have a refresh button to refetch things (but preserving the order) how we do that?
    • we need to know that the records are in Loading state, and in that case we don't include a model
    • we then rebuild the table model when the records arrived
    • it works but is odd that we need to leave the table model out of sync. we could at least rebuild it with an empty list of entries...

But if the List entry becomes a Maybe (List entry) we can rebuild things more easily and straight forward from a RemoteData (as anything that is not Succeed is mapped to Nothing).

(Slightly odd that the loading state don't show the arrows in the table header)

Not 100% sure. WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah I think this is a good idea.

@michaelglass michaelglass force-pushed the faster-sortable-table branch 6 times, most recently from 21dab09 to 6ed4544 Compare March 25, 2025 11:45
@michaelglass michaelglass force-pushed the faster-sortable-table branch from 6ed4544 to 3391cda Compare March 25, 2025 12:00
Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@michaelglass michaelglass added this pull request to the merge queue Mar 25, 2025
Merged via the queue into master with commit 60ccfda Mar 25, 2025
7 checks passed
@michaelglass michaelglass deleted the faster-sortable-table branch March 25, 2025 16:01
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.

None yet

2 participants