frinyvonnick / react-simple-infinite-loading

A list that infinitely loads content as user scrolls down in React
https://www.npmjs.com/package/react-simple-infinite-loading
Apache License 2.0
59 stars 13 forks source link

Pass down a itemsCount property #28

Closed frinyvonnick closed 5 years ago

frinyvonnick commented 5 years ago

react-simple-infinite-loading handles the itemsCount property from react-window-infinite-loader to simplify its usage for infinite loading streams like Twitter or Facebook feeds. Sometimes you already know the items count. It would be nice to add an itemsCount property to InfiniteLoader component to avoid having the scrollbar changing its size as the items being loaded.

gVirtu commented 5 years ago

I'd like to take a stab at this one! :) Looks simple enough, I'll have something up shortly and will let you know.

frinyvonnick commented 5 years ago

Thank you @gVirtu for considering contributing to react-simple-infinite-loading 🙏 Ask if you have any questions 👍

gVirtu commented 5 years ago

I'm really liking how clean the code currently is and I'd love to keep it that way! So I would like to ask your opinion on which approach looks better:

Currently the itemsCount is calculated as

const itemsCount = hasMoreItems ? children.length + 1 : children.length

Now, by adding an optional itemsCount prop, we make sure it overrides this calculation when applicable, so I was thinking either:

  1. Having it as a nested ternary. Simple enough, but is it legible according to our standards?

    const effectiveCount = (itemsCount !== undefined) 
                       ? (itemsCount) 
                       : (hasMoreItems ? children.length + 1 : children.length)
  2. Factoring out this logic to a function. Though I noticed we have no other utility functions so far, so this may be overkill.

    function getEffectiveCount(itemsCount, hasMoreItems, childrenLength) {
    if (itemsCount === undefined)
    return hasMoreItems ? childrenLength + 1 : childrenLength
    return itemsCount
    }

Just to make sure this tiny improvement meets the project's long term expectations. :) Thanks!

frinyvonnick commented 5 years ago

@gVirtu thank you for the compliment ❤️

Really interesting propositions, I prefer the second one. I have a third approach which imho is halfway of yours:

let effectiveCount = itemsCount 
if (effectiveCount === undefined) {
    effectiveCount = hasMoreItems ? childrenLength + 1 : childrenLength
}

It avoids having nested ternaries. What do you think of it?

gVirtu commented 5 years ago

I like it! Simple and straightforward. I'll go with it. Thanks for the input!