alsoscotland / react-super-select

MIT License
95 stars 33 forks source link

Paging quirks #143

Closed TimNZ closed 6 years ago

TimNZ commented 6 years ago

Is it intentional that you need to implement both ajaxDataFetch and pageDataFetch for paging?

If I DO NOT implement ajaxDataFetch, no page load is triggered on dropdown open until I move the mouse over the dropdown container.

If I DO implement ajaxDataFetch, it is called twice immediately on initial open, twice being one more time than necessary :)

I then have to implement hacky code to ensure I don't increase my nextPage counter in my shared loadData() if it is another call triggered from ajaxDataFetch.

screen shot 2018-03-02 at 11 04 53 am
alsoscotland commented 6 years ago

@TimNZ sounds like it is probably a bug where the paging handler is being called due there not being content in the drop down. It basically gets triggered anytime the bottom of the dropdown list becomes visible by scrolling

alsoscotland commented 6 years ago

@TimNZ I added a potential fix to https://github.com/alsoscotland/react-super-select/tree/v1.0.12 you will still have to defined both if the intent is to ajax fetch the first set of content. But I think this will prevent the double fetch

TimNZ commented 6 years ago

Thanks for the fast turn-around.

No change.

_fetchDataViaAjax has one call path: -> _getOptionsMarkup -> _getDropdownContent -> render()

render() is called multiple times in the initial render lifecycle and state.rawDataSource is not set before _needsAjaxFetch is called multiple times, and ajaxDataFetch() is of course async allowing this to happen.

You could debug the lifecycle to see how to do things alternatively, or do what I did as a hack, your call.

For testing I cloned the src and set 'this.doneAjax = true' in _fetchDataViaAjax and changed the needsAjaxFetch check accordingly.

  _fetchDataViaAjax() {
    const self = this;
    this.doneAjaxFetch = true
    this.props.ajaxDataFetch(this.state.rawDataSource).then((dataSourceFromAjax) => {
   ...

  _needsAjaxFetch() {
    // return (isUndefined(this.state.rawDataSource) && isFunction(this.props.ajaxDataFetch));
    return (!this.doneAjaxFetch && isFunction(this.props.ajaxDataFetch));
  }
alsoscotland commented 6 years ago

@TimNZ Okay. I took the issue to be that the paging and the ajax fetch were both being called. tried a quick fix for that. I will take another look.

TimNZ commented 6 years ago

Thanks.

Having to implement both func is a separate consideration.

I don't find it to be an intuitive implementation choice, and it is not clear in the documentation or from the example how to do paging and that Both handlers are required, so I had to spend a bit of time sussing out the flow. Which led to seeing ajaxDataFetch was called twice.

Every ajax mechanism I've used or implemented has one func handler. It would be simpler for you to maintain and for consumers to just have ajaxDataFetch and pass additional useful hint params, and see how you deprecate pageDataFetch, and see if you need additional props to control execution.

The return from ajaxDataFetch could indicate if more data is available.

alsoscotland commented 6 years ago

I agree that it is not ideal. It grew out of two distinct use cases. page fetch from where you have the first page and the ajax for subsequent pages, the other where you made a single ajax fetch for a single page of results.

alsoscotland commented 6 years ago

@TimNZ Spent some time earlier tonight investigating the double renders.
Unfortunately, they are due to the tracking of focused option id in component state.

I appreciate you taking the time with the control and your points are valid. Consolidating the ajax handlers and deprecating the separate paging setup would be ideal, but at the moment I do not have the bandwidth to investigate further.

I did implement a version of a boolean similar to your doneAjaxFetch solution above to prevent additional fetch calls https://github.com/alsoscotland/react-super-select/tree/v1.0.13

TimNZ commented 6 years ago

I'll see how I feel about doing a PR next week.

alsoscotland commented 6 years ago

@TimNZ Going to close this for now