Feature: Debounce paging #178
Conversation
| @@ -0,0 +1,80 @@ | |||
| /** | |||
| * TODO | |||
|
@EWhite613 was |
|
This behavior is not a true debouncer, though it exhibits some behaviors of one. A debouncer limits the rate at which a function can fire. Attaching a debouncer to the window resize event is a good example where you can have multiple inputs actions/events but only want one execution to occur. In the logic you have put forward we are debouncing in the sense that we are not going to fire off intermediate data requests for each data page HOWEVER we then jump the user forward as many data pages as they have queued up so we are executing part of the logic more than once (data page selection vs actual retrieval). The point: 1) we may wish to use a more accurate term and/or describe this behavior more accurately, and 2) this seems like it would be a jarring experience for the user. Especially as they click the next page and are waiting for results, don't get any, and then start clicking a bunch wondering what is broken/going on. If the interval value is kept to a low number though it will likely lessen this occurrence but if that is how this is intended to be mitigated this guidance should be conveyed in the documentation. |
| let newPage = currentPage + counter | ||
| newPage = newPage < 0 ? 0 : newPage | ||
| newPage = newPage > totalPages ? totalPages : newPage | ||
| this.set('_debouncedPaging', run.debounce(this, this._sendDebounceOnChange, newPage, debounceInterval)) |
There was a problem hiding this comment.
Multiple this.set() calls should be combined into a single this.setProperties() call.
| const totalPages = this.get('_lastPage') | ||
| let counter = this.get('pageClickCounter') || 0 | ||
| const isGoingBack = currentPage > page | ||
| isGoingBack ? counter-- : counter++ |
There was a problem hiding this comment.
Prefer to see these two lines combined as:
(currentPage > page) ? counter-- : counter++
If the const was used to provide context to the logic then a comment is preferred instead of a construct.
| this._sendDebounceOnChange(page) | ||
| } else { | ||
| const totalPages = this.get('_lastPage') | ||
| let counter = this.get('pageClickCounter') || 0 |
There was a problem hiding this comment.
Use this.getWithDefault('pageClickCounter', 0) instead.
Line 36 has set _pageClickCounter to 0 but then it is never used in the code anywhere. I'm guessing that it should have not had an underscore and then it's value would have been what was expected here and having to default it to 0 wouldn't even be necessary?
| export default Component.extend({ | ||
|
|
||
| // == Dependencies ========================================================== | ||
|
|
||
| layout, | ||
|
|
||
| // == Properties ============================================================ | ||
|
|
||
| _pageClickCounter: 0, |
There was a problem hiding this comment.
Never used anywhere. See https://github.com/ciena-frost/ember-frost-list/pull/178/files#r168236617 and the either refactor or remove.
| }, | ||
|
|
||
| onPaginationChange (page) { | ||
| this.get('notifications').success(`recieved action with page ${page}`, { |
There was a problem hiding this comment.
Change recieved to received
|
Looking into different approach |
This project uses semver, please check the scope of this pr:
CHANGELOG