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

Paging quirks #143

Closed
TimNZ opened this issue Mar 1, 2018 · 9 comments
Closed

Paging quirks #143

TimNZ opened this issue Mar 1, 2018 · 9 comments

Comments

@TimNZ
Copy link
Contributor

TimNZ commented Mar 1, 2018

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
Copy link
Owner

alsoscotland commented Mar 2, 2018

@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
Copy link
Owner

@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
Copy link
Contributor Author

TimNZ commented Mar 3, 2018

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 alsoscotland reopened this Mar 3, 2018
@alsoscotland
Copy link
Owner

@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
Copy link
Contributor Author

TimNZ commented Mar 3, 2018

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
Copy link
Owner

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
Copy link
Owner

@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
Copy link
Contributor Author

TimNZ commented Mar 5, 2018

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

@alsoscotland
Copy link
Owner

@TimNZ Going to close this for now

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

No branches or pull requests

2 participants