day8 / re-com

A ClojureScript library of reusable components for Reagent
https://re-com.day8.com.au
MIT License
797 stars 146 forks source link

Does `v-table` need `:id-fn` ? #243

Closed mike-thompson-day8 closed 3 years ago

mike-thompson-day8 commented 3 years ago

What purpose does :id-fn serve in v-table? The component itself seems to work principally with row-index and the elements of :model

Can we remove it? Should we change it?

Gregg's initial answer:

It's primary purpose is to provide a unique id for React, just like in any of our other 
re-com components that render lists of things.
Note that row-index was added later, so perhaps we can revisit using row-index as unique ids. 
I would have to look further into it

My guess is that we can probably move to row_index. That would make scrolling efficient (aka adding and removing rows). However in apps where the underlying :model is changing (items are being added and removed often), then letting the programmer control this would be a good idea. Its just that this is an edge case I think.

Proposal:

  1. Gregg checks if :id-fn is only used in this one way (for React keys).
  2. If so, we keep :id-fn but we:
    • document that's the purpose and change the name to reflect its real purpose: :key-fn
    • make it optional
    • make it take two args row_index and row
    • provide a default which returns the row-index arg
Gregg8 commented 3 years ago
  1. Yep, only used for React keys
  2. Done (except the code determines if :key-fn needs to be called, so no need to pass it two args)