Skip to content
This repository was archived by the owner on Aug 21, 2023. It is now read-only.

Conversation

@ItsJonQ
Copy link
Contributor

@ItsJonQ ItsJonQ commented Oct 11, 2017

Add InfiniteScroller component + enhance Scrollable

This update adds a new InfiniteScroller component that makes it easy to dynamically fetch/load/inject content (or do anything else!). It has a couple of handy callbacks to provide the user with control when the component is scrolled into position.

Example

<Scrollable>
  ...
  <InfiniteScroller onLoading={loadMoreContent} />
</Scrollable>

The Scrollable component has also been enhanced with a new onScroll callback.

Additional Updates

  • Adjust markup for Modal to better support Scrollable usage
  • Add scrollParent prop to InfiniteScroller to accept DOM elements
  • Create new Storybook for InfiniteScroller example (with Modal)

Other Updates

  • Remove yarn.lock (and add to gitignore)
  • Remove fetch dependency and polyfill. Blue doesn't use Fetch API.
  • Add prop-types-extra for DOM node checking

Resolves: #12

This update adds a new InfiniteScroller component that makes it easy to
dynamically fetch/load/inject content (or do anything else!). It has a couple
of handy callbacks to provide the user with control when the component is
scrolled into position.

The Scrollable component has also been enhanced with a new `onScroll` callback.
@ItsJonQ ItsJonQ self-assigned this Oct 11, 2017
@ItsJonQ ItsJonQ requested a review from brettjonesdev October 11, 2017 18:29
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3895e5c on infinite-scroll into a6a21d5 on master.

// This is a super fail-safe. This will always be parentNode, with the
// exception of document or window. Cannot be tested in JSDOM/Enzyme,
// since it prohibits mounting on document.body directly.
const nodeScope = node && node.parentNode ? node.parentNode : window
Copy link
Contributor

Choose a reason for hiding this comment

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

@ItsJonQ Does this mean that the InfiniteScroll will only work if it is the direct child of a Scrollable? Or rather, if its parent is styled such that it can be scrolled?

Copy link
Contributor Author

@ItsJonQ ItsJonQ Oct 11, 2017

Choose a reason for hiding this comment

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

InfiniteScroller watches scroll for any parent container, not just the Scrollable component.

And correct, it only triggers on scroll.

I can add 2 new options…

A scope prop to watch for scroll of a specific node. But I can't imagine this component listening for anything that isn't the parent node. So this may not be as useful.

Also a loadOnMount, which does the check when this component is mounted vs only listening for scroll.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I wasn't clear. My concern is that I've got something like this:

  <Scrollable>
     <Conversation>
      <ThreadList>
        <InfiniteScroll />
      </ThreadList>
    </Conversation>
  </Scrollable>
</Modal>

In this case, doesn't the scroll event fire on the Scrollable? And won't that mean that the <InfiniteScroll> will miss it, since it's only listening for its direct parent to have a scroll event?

Copy link
Contributor

@brettjonesdev brettjonesdev Oct 12, 2017

Choose a reason for hiding this comment

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

OK so I went ahead and played with this, I have some work locally which add a infiniteScroller prop to the <Modal> so that I should be able to pass in an InfiniteScroller of my choosing to the Conversation's <Modal> in beacon2. I'm really unsure if this is the right approach, though, because I don't like having to be aware of scrolling at the level of the <Modal>? Like, I really want to put the InfiniteScroll down inside my ThreadList or something...

Seems to me like we can either do what I was working towards, adding an infiniteScroller prop to the Modal - or - make the logic which "finds" the parentElement overridable, so that we could pass a custom function in like <InfiniteScroll getScrollParent={getScrollParent}/>.

I'm kinda leaning towards the latter, what do you think? The latter would allow me to pass a getScrollParent prop down from the modal into the ThreadList, so it would have all the logic it needs for infinite scroll, with an abstract scroll parent that the Modal knows about. What do you think?

Copy link
Contributor Author

@ItsJonQ ItsJonQ Oct 12, 2017

Choose a reason for hiding this comment

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

Updates! I've added a new scrollParent prop to InfiniteScroller. You can pass any DOM node there, and the component will listen to scroll events on that instead. Otherwise, it will find it's closest parentNode.

I've also adjusted the Modal markup to make it more <Scrollable> friendly.

Is it possible for the thing you're building to have this structure?

<Modal trigger={myTrigger}>
  <Conversation>
    <ThreadList>
      ...
    </ThreadList>
  </Conversation>
  <InfiniteScroller />
</Modal>

Edit: Below example doesn't seem to work… the ref isn't taking. Testing now

If so, then InfiniteScroller would just automatically work. If not… then you can maybe do something like this?

<Modal trigger={myTrigger}>
  <Conversation>
    <ThreadList ref={{node => this.threadListNode = node}}>
      <InfiniteScroll scrollParent={this.threadListNode} />
    </ThreadList>
  </Conversation>
</Modal>

@ItsJonQ
Copy link
Contributor Author

ItsJonQ commented Oct 11, 2017

Note for enhancement: Support load for scroll direction up. It may be supported? But not tested. It is currently only tested when scrolling down.

ItsJonQ added 2 commits October 12, 2017 12:39
**Main Updates**
* Adjust markup for Modal to better support Scrollable usage
* Add scrollParent prop to InfiniteScroller to accept DOM elements
* Create new Storybook for InfiniteScroller example (with Modal)

**Other Updates**
* Remove yarn.lock (and add to gitignore)
* Remove fetch dependency and polyfill. Blue doesn't use Fetch API.
* Add prop-types-extra for DOM node checking
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 147a835 on infinite-scroll into a6a21d5 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c180708 on infinite-scroll into a6a21d5 on master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c180708 on infinite-scroll into a6a21d5 on master.

Copy link
Contributor

@brettjonesdev brettjonesdev left a comment

Choose a reason for hiding this comment

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

Looks great! This is exactly what I think I'll need to get scrolling going inside Modals.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 3708767 on infinite-scroll into a6a21d5 on master.

@ItsJonQ ItsJonQ merged commit bb4a623 into master Oct 12, 2017
@ItsJonQ ItsJonQ deleted the infinite-scroll branch October 12, 2017 18:32
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants