Fetch next page of facilities via infinite scroll component #750

kellyi · 2019-08-26T22:21:37Z

Overview

Adjust facilities tab so that it will fetch the next page of facilities
on reaching the bottom of the infinite scroll list.

Also:

remove some now unused utility code & tests
update facilities list endpoint to return results in alphabetical
order
update url to retrieve facilities to request pages of 25 results at a
time
add a loading indicator to display contextually at the temporary bottom of the list when there's another page of items to fetch

Connects #742

Notes

I'm going to make a new issue for scroll restoration. I had worked out part of a solution using a custom hook, but it's most likely an enhancement that we can address later if desired.

Testing Instructions

serve this branch, then search for some facilities
verify that the results return in alphabetical order in the sidebar and that a new page is fetched each time you reach the bottom of the list (until no pages remain)
verify that any filters you've applied remain applied when fetching a new page of results

Checklist

fixup! commits have been squashed
CI passes after rebase
CHANGELOG.md updated with summary of features or fixes, following Keep a Changelog guidelines

kellyi · 2019-08-27T21:28:54Z

Targeted this at the marker click branch, btw, since I initially branched off that one.

jwalgran · 2019-08-27T22:12:23Z

Looking at this now.

jwalgran

Nice implementation. I'm glad this was as simple as I expected, and I love a PR that adds functionality and decreases line count at the same time. I have some small questions and suggestions.

jwalgran · 2019-08-27T22:15:12Z

src/app/src/actions/facilities.js

+        } = getState();
+
+        if (!nextPageURL) {
+            return noop();


Is there a benefit to return noop(); instead of the equivalent return;?

I believe they're basically equivalent insofar as each will make the return value undefined, but I think the explicit return noop() is a little more readable when scanning through the code.

jwalgran · 2019-08-27T22:35:23Z

src/app/src/components/FilterSidebarFacilitiesTab.jsx

-    const headerDisplayString = facilitiesCount && (facilitiesCount !== filteredFacilities.length)
-        ? `Displaying ${filteredFacilities.length} facilities of ${facilitiesCount} results`
-        : `Displaying ${filteredFacilities.length} facilities`;
+    const headerDisplayString = facilitiesCount && `${facilitiesCount} results`;


0 is falsy, so headerDisplayString would be "0" instead of "0 results." We currently don't show the results list if there are zero results, so this isn't really an issue.

Consider adding a pluralization util function to avoid showing "1 results"

Added a utility function and test in 915004a

jwalgran · 2019-08-27T22:38:19Z

src/app/src/util/util.js

@@ -93,7 +91,7 @@ export const makeGetCountriesURL = () => '/api/countries/';

 export const makeGetFacilitiesURL = () => '/api/facilities/';
 export const makeGetFacilityByOARIdURL = oarId => `/api/facilities/${oarId}/`;
-export const makeGetFacilitiesURLWithQueryString = qs => `/api/facilities/?${qs}`;
+export const makeGetFacilitiesURLWithQueryString = qs => `/api/facilities/?${qs}&pageSize=25`;


We check this literal 25 in a test so we should consider extracting it into constants.js

Sounds good! Done in 1de1e06

jwalgran · 2019-08-27T22:41:41Z

src/app/src/reducers/FacilitiesReducer.js

+            error: { $set: null },
+            data: {
+                features: {
+                    $set: state.facilities.data.features.concat(get(payload, 'features', [])),


Consider

$push: get(payload, 'features', [])

Which appears to work identically

Cool. This is
3be94f6

kellyi · 2019-08-28T15:52:38Z

@jwalgran I believe this is ready for another look!

Adjust facilities tab so that it will fetch the next page of facilities on reaching the bottom of the infinite scroll list. Also: - remove some now unused utility code & tests - update facilities list endpoint to return results in alphabetical order - update url to retrieve facilities to request pages of 25 results at a time - add a loading indicator contextually if there's a next page of facilities to fetch

kellyi · 2019-08-28T17:40:48Z

Squashed and rebased on develop.

jwalgran

This is working perfectly, but it occurred to me that this is probably not the behavior we want when the vector tile feature flag is off

I had imagined that the entire "show arbitrary number of facilities" feature would be enabled as a whole. Do you think we could gate this new behavior with the feature flag without a burdensome amount of rewriting?

kellyi · 2019-08-29T16:06:22Z

Yep, feature flagging this should be easy. In this case what I will do is just add a temporary component that is named NonVectorTileFilterSidebarFacilitiesTab and just includes the old code, then set things up to display conditionally based on whether the feature flag is set.

kellyi · 2019-08-29T16:40:50Z

@jwalgran 73ad944 adds back the old facilities sidebar functionality so that we can toggle the behavior with the vector_tile switch. I opted to rename the old component rather than the new one, since the download CSV PR has some changes targeting the new component.

jwalgran

Thanks for quickly implementing the feature flag. Works perfectly.

Restore the old sidebar facilities tab as a `NonVectorTileFilterSidebarFacilitiesTab` component so that it can be displayed when the vector_tile flag is off. Restore some utility functions (moved into the NonVectorTile... component) which were required to make the component work, along with some Redux actions it expected to exist. Adjust the fetchFacilities function to accept an options object so that callers can set the page request size. We don't need to make a similar adjustment to the fetchNextPage function since it uses the URL returned from the API for its next page of results.

kellyi · 2019-08-29T18:44:22Z

Thanks for your help with this!

kellyi force-pushed the ki/load-facilities-on-sidebar-scroll branch 4 times, most recently from 0fb908c to 72e7a41 Compare August 27, 2019 21:26

kellyi marked this pull request as ready for review August 27, 2019 21:28

kellyi requested a review from jwalgran August 27, 2019 21:29

kellyi assigned jwalgran Aug 27, 2019

jwalgran reviewed Aug 27, 2019

View reviewed changes

jwalgran assigned kellyi Aug 27, 2019

kellyi force-pushed the ki/load-facilities-on-sidebar-scroll branch from aae6a11 to 915004a Compare August 28, 2019 15:51

kellyi force-pushed the ki/handle-marker-clicks branch from 7fe7539 to 2a87040 Compare August 28, 2019 17:26

kellyi changed the base branch from ki/handle-marker-clicks to develop August 28, 2019 17:38

kellyi force-pushed the ki/load-facilities-on-sidebar-scroll branch from 76c4b2c to bee55de Compare August 28, 2019 17:40

jwalgran reviewed Aug 29, 2019

View reviewed changes

kellyi changed the title ~~Fetch next page of facilities via infinite scroll~~ Fetch next page of facilities via infinite scroll component Aug 29, 2019

kellyi mentioned this pull request Aug 29, 2019

Enable scroll restoration with infinite scroll component #753

Open

jwalgran approved these changes Aug 29, 2019

View reviewed changes

jwalgran removed their assignment Aug 29, 2019

kellyi force-pushed the ki/load-facilities-on-sidebar-scroll branch from 7f44a17 to 3058053 Compare August 29, 2019 18:39

kellyi merged commit e019541 into develop Aug 29, 2019

kellyi deleted the ki/load-facilities-on-sidebar-scroll branch August 29, 2019 18:47

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch next page of facilities via infinite scroll component #750

Fetch next page of facilities via infinite scroll component #750

kellyi commented Aug 26, 2019 •

edited

kellyi commented Aug 27, 2019

jwalgran commented Aug 27, 2019

jwalgran left a comment

jwalgran Aug 27, 2019

kellyi Aug 28, 2019

jwalgran Aug 27, 2019

kellyi Aug 28, 2019

jwalgran Aug 27, 2019

kellyi Aug 28, 2019

jwalgran Aug 27, 2019

kellyi Aug 28, 2019

kellyi commented Aug 28, 2019

kellyi commented Aug 28, 2019

jwalgran left a comment

kellyi commented Aug 29, 2019

kellyi commented Aug 29, 2019

jwalgran left a comment

kellyi commented Aug 29, 2019

Fetch next page of facilities via infinite scroll component #750

Fetch next page of facilities via infinite scroll component #750

Conversation

kellyi commented Aug 26, 2019 • edited

Overview

Notes

Testing Instructions

Checklist

kellyi commented Aug 27, 2019

jwalgran commented Aug 27, 2019

jwalgran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyi commented Aug 28, 2019

kellyi commented Aug 28, 2019

jwalgran left a comment

Choose a reason for hiding this comment

kellyi commented Aug 29, 2019

kellyi commented Aug 29, 2019

jwalgran left a comment

Choose a reason for hiding this comment

kellyi commented Aug 29, 2019

kellyi commented Aug 26, 2019 •

edited