Create facilities tab & display text search results #237

kellyi · 2019-02-28T22:18:43Z

Overview

This PR adds a facilities list tab which will display a list of text search results as depicted below when the facilities results return from a search of the API. I also added a free text search which currently searches on the name address and country_name fields to narrow down the list of visible results, as well as a "Download CSV" functionality which downloads a list of CSV items.

Additionally, this PR also:

replaces papaparse with a custom utility function to turn a 2d array into a string, which circumvents a bug whereby JSON.parse would sometimes try to parse the OAR ID as a number but fail and instead print NaN to the CSV
adds tests for that newly added utility function
adjusts the contributor list to list them in alphabetical order by name
adjusts the app's DOM structure to make it possible to scroll subcomponents rather than scrolling the entire app
changes the app name in package.json from "redpanda" (which was the legacy name?) to "open apparel registry"

Connects #209
Connects #227
Connects #127
Connects #123

Demo

CSV:
facilities.csv.txt

No results for a search:

Notes

I made #244 for the map interactions because it will need some dedicated thought about how to handle the various possible kinds of interaction.

Testing

get this branch then ./scripts/update
run ./scripts/test and verify that everything passes
./scripts/resetdb
visit the main map page and
- verify that the contributors are now listed in alphabetical order by name
- make a search and verify that you are navigated to the facilities tab on success
- verify that the free text search works as expected
- verify that you can download a list of facilities, filtered by your free text search
- verify that you can scroll through the facilities and that clicking on any of the list items takes you to its details route
on the Macbook-sized viewport, check that the following pages scroll correctly following the changes I made to the DOM:
- /auth/register
- /contribute
- /profile/:id/
- /lists
- /lists/:id
download a facility list and verify that that still works, following the changes to replace papaparse

jwalgran · 2019-03-04T19:35:02Z

Looking at this now

jwalgran · 2019-03-04T19:51:06Z

I'm seeing an odd click target bug when switching tabs in Chrome. Only clicks at the very top or bottom edges of the tab are triggering the state change. Clicks in the middle of the tab are triggering the Material UI animation, but not a state change.

kellyi · 2019-03-04T20:01:49Z

I'm seeing an odd click target bug when switching tabs in Chrome.

Weird! I wonder if I set up the click handlers incorrectly. I tested staging and it's not happening there.

kellyi · 2019-03-04T20:04:44Z

Interesting, I must need to add an ID to another element:

kellyi · 2019-03-04T20:11:37Z

Turns out the correct function signature is:

  handleChange = (event, value) => {
    this.setState({ value });
  };

Adjusted it in 1a05bab

jwalgran · 2019-03-04T20:25:02Z

It is a bit frustrating to loose scroll state on the search results. When navigating back from the details view. Have we done any scroll restoration on other projects that could possibly apply here? This react-router document talks about the issue but does not include a general solution for our issue. https://github.com/ReactTraining/react-router/blob/master/packages/react-router-dom/docs/guides/scroll-restoration.md

kellyi · 2019-03-04T20:31:19Z

It is a bit frustrating to loose scroll state on the search results. When navigating back from the details view. Have we done any scroll restoration on other projects that could possibly apply here?

Make sense. I haven't ever worked on it; the nearest thing to this is the parcel cards sidebar in GARP for which we declined to try to set up scroll restoration.

I'm happy to look into it, but I wonder if it'd make sense as a new card?

jwalgran · 2019-03-04T20:33:20Z

I'm happy to look into it, but I wonder if it'd make sense as a new card?
Yes. We can break scroll state restoration into a separate issue.

jwalgran

I like the design of this solution and it is working well. I made a few small suggestions inline.

There is one usability/inconsistency issue that we may need to consider. There are currently two ways to get to the search results:

Clicking the "Search" button on the Search tab.
Clicking the "Facilities" tab.
If you run a search, click back to the search tab, change one of the filters, then click back to the "Facilities" tab you are now in a kind of inconsistent state. We may want to consider one of the following changes to make sure the state of the two tabs are always consistent.
Disable the facilities tab (make it inaccessible) until a search has been run
Make clicking on the facilities tab trigger a search if the search options have changed
Remove the search button and always have the results on facilities tab match the search fields

I think making a click on the facilities tab trigger the search if the options have changed is the most natural given the current behavior. Do you have opinions or other ideas?

jwalgran · 2019-03-04T19:38:25Z

src/app/package.json

@@ -40,8 +38,7 @@
        "build": "react-scripts build",
        "test": "CI=1 react-scripts test",
        "lint": "eslint src/ --ext .js --ext .jsx",
-        "eject": "react-scripts eject",
-        "deploy": "npm run build && firebase deploy"


It's good that we are removing this legacy deploy command, but we should consider documenting in the commit message why we making this change that is unrelated to CSV processing.

Just a note: I have not changed this yet but I will update the commit message when squashing and rebasing before I merge.

Updated commit is
6fd1a9b

jwalgran · 2019-03-04T19:39:54Z

src/app/src/__tests__/utils.tests.js

+
+    const stringArray = [
+        [
+            'hello',


We should consider including one of our pathological hex strings in the tests.

Good idea! I added the problematic one in 5c02862

jwalgran · 2019-03-04T19:57:30Z

src/app/src/components/FilterSidebarFacilitiesTab.jsx

+                <label
+                    htmlFor={SEARCH_TERM_INPUT}
+                >
+                    Search results


We should consider changing this label to "Filter results" to distinguish search, which takes place on the "Search" tab, from narrowing the list with a text filter. We may want to carry this renaming through to code constants too, i.e. SEARCH_TERM_INPUT -> FILTER_TERM_INPUT

Sounds good. 42bbd93 renames these from using searchTerm to using filterText.

I am still seeing "Search" on the "Facilities" tab. Consider changing the label to "Filter results" and the placeholder text to "Filter by name, address, or country"

I replaced the "Search" text with "Filter" in the adjusted last commit here.

jwalgran · 2019-03-04T20:39:35Z

src/app/src/components/FilterSidebarFacilitiesTab.jsx

+
+const SEARCH_TERM_INPUT = 'SEARCH_TERM_INPUT';
+
+const makeFacilityListHeightPercentageString = () => {


On my 1680x1050 screen there is some whitespace between the scrolling results list and the footer. If I shrink the browser window height the whitespace goes away. I looks like the conditional styling set up by this function may be the reason. Is there a way we can make sure that the scrolling area always fills the container?

kellyi · 2019-03-04T20:52:18Z

If you run a search, click back to the search tab, change one of the filters, then click back to the "Facilities" tab you are now in a kind of inconsistent state. We may want to consider one of the following changes to make sure the state of the two tabs are always consistent.

How would you feel about dumping any existing facilities data altogether when the state of the filters on the search tab changes? That'd also solve an inconsistency between prior data and the new filters before "search" has been clicked.

jwalgran · 2019-03-04T20:58:01Z

How would you feel about dumping any existing facilities data altogether when the state of the filters changes?

I think that is a good idea in general, but what would the facilities tab say at that point? If it reverts to the "No facilities were found for that search" message we are in the same situation.

kellyi · 2019-03-04T22:45:57Z

I think that is a good idea in general, but what would the facilities tab say at that point? If it reverts to the "No facilities were found for that search" message we are in the same situation.

Interesting. Maybe we just don't show the tab at all if there are no facilities in the state to display? I'll try it out locally to see how that works.

kellyi · 2019-03-05T16:33:42Z

9a189ee creates a new "no facilities to display" state for the sidebar tab so that it will look like this when there's nothing to show:

In addition, I updated the application state to clear facilities data when any of the search filters change and to drop the markers from the map when the facilities data is cleared. For the latter, I kept the layer around, but just replace the data with an empty geojson feature collection so that no markers will show.

kellyi · 2019-03-05T17:15:44Z

cd088df updates the facilities list to have its height set dynamically based on the viewport size

@jwalgran this is ready for another look!

kellyi · 2019-03-05T17:31:24Z

#250 is for scroll restoration

It occurred to me that we may be able to find an infinite scroll library that supports scroll restoration innately. That would also solve potential performance problems if the list of search results was in the thousands.

- add an order_by('name') clause to the queryset in all_contributors to sort the results so they'll display alphabetically by name in the client Connects #227

- remove papaparse library - create utility function for generating CSV row strings in order to circumvent a bug whereby JSON.parse would occasionally cast OAR ID shas into numbers - add tests for CSV parsing function - rename app from "redpanda" to "open apparel registry" in package.json - drop legacy "npm deploy" script from package.json

kellyi · 2019-03-05T19:32:42Z

Rebased this on develop & squashed.

jwalgran · 2019-03-05T19:44:24Z

Looking at this now

jwalgran

This looks and works well. Thanks for working through the issues. I made just one small text suggestion.

jwalgran · 2019-03-05T21:06:23Z

src/app/src/components/FilterSidebarFacilitiesTab.jsx

+                <label
+                    htmlFor={SEARCH_TERM_INPUT}
+                >
+                    Search results


I am still seeing "Search" on the "Facilities" tab. Consider changing the label to "Filter results" and the placeholder text to "Filter by name, address, or country"

kellyi · 2019-03-05T21:14:30Z

I am still seeing "Search" on the "Facilities" tab. Consider changing the label to "Filter results" and the placeholder text to "Filter by name, address, or country"

Oof, must have forgot to update this. Will make this change before merging.

- create facilities tab for displaying a text list of facilities - add free text search field to search on name, and country name fields - add functionality to download a CSV of facilities filtered by free text search - update Redux actions to handle toggling & displaying the facilities tab and storing the free text search value - adjust application HTML element hierarchy & style sheet in order to - prevent the enter app from scrolling vertically - enable select components to scroll vertically when necessary

kellyi · 2019-03-05T21:22:04Z

Thanks for your help with this!

kellyi added the in progress label Feb 28, 2019

kellyi force-pushed the ki/list-facilities branch 6 times, most recently from bf31713 to de75e16 Compare March 1, 2019 22:30

kellyi force-pushed the ki/list-facilities branch 2 times, most recently from 6de75f1 to 12ed908 Compare March 4, 2019 16:37

kellyi changed the title ~~WIP: Create facilities tab & display text search results~~ Create facilities tab & display text search results Mar 4, 2019

kellyi marked this pull request as ready for review March 4, 2019 16:52

kellyi requested a review from jwalgran March 4, 2019 16:52

kellyi assigned jwalgran Mar 4, 2019

jwalgran reviewed Mar 4, 2019

View reviewed changes

jwalgran assigned kellyi Mar 4, 2019

Kelly Innes added 2 commits March 5, 2019 14:17

Sort all_contributors alphabetically by name

1703ac3

- add an order_by('name') clause to the queryset in all_contributors to sort the results so they'll display alphabetically by name in the client Connects #227

kellyi force-pushed the ki/list-facilities branch from cd088df to 38881b9 Compare March 5, 2019 19:19

jwalgran approved these changes Mar 5, 2019

View reviewed changes

jwalgran removed their assignment Mar 5, 2019

kellyi force-pushed the ki/list-facilities branch from 38881b9 to b9d88e6 Compare March 5, 2019 21:15

kellyi merged commit 57a1cbe into develop Mar 5, 2019

hectcastro removed the in progress label Mar 5, 2019

kellyi deleted the ki/list-facilities branch March 5, 2019 21:22

kellyi mentioned this pull request Mar 5, 2019

Update react-scripts #247

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Create facilities tab & display text search results #237

Create facilities tab & display text search results #237

kellyi commented Feb 28, 2019 •

edited

Loading

jwalgran commented Mar 4, 2019

jwalgran commented Mar 4, 2019

kellyi commented Mar 4, 2019

kellyi commented Mar 4, 2019

kellyi commented Mar 4, 2019

jwalgran commented Mar 4, 2019

kellyi commented Mar 4, 2019

jwalgran commented Mar 4, 2019

jwalgran left a comment

jwalgran Mar 4, 2019

kellyi Mar 5, 2019

kellyi Mar 5, 2019

jwalgran Mar 4, 2019

kellyi Mar 4, 2019

jwalgran Mar 4, 2019

kellyi Mar 4, 2019

jwalgran Mar 5, 2019

kellyi Mar 5, 2019

jwalgran Mar 4, 2019

kellyi commented Mar 4, 2019 •

edited

Loading

jwalgran commented Mar 4, 2019 •

edited

Loading

kellyi commented Mar 4, 2019

kellyi commented Mar 5, 2019

kellyi commented Mar 5, 2019

kellyi commented Mar 5, 2019

kellyi commented Mar 5, 2019

jwalgran commented Mar 5, 2019

jwalgran left a comment

jwalgran Mar 5, 2019

kellyi commented Mar 5, 2019

kellyi commented Mar 5, 2019


		const SEARCH_TERM_INPUT = 'SEARCH_TERM_INPUT';

		const makeFacilityListHeightPercentageString = () => {

Create facilities tab & display text search results #237

Create facilities tab & display text search results #237

Conversation

kellyi commented Feb 28, 2019 • edited Loading

Overview

Demo

Notes

Testing

jwalgran commented Mar 4, 2019

jwalgran commented Mar 4, 2019

kellyi commented Mar 4, 2019

kellyi commented Mar 4, 2019

kellyi commented Mar 4, 2019

jwalgran commented Mar 4, 2019

kellyi commented Mar 4, 2019

jwalgran commented Mar 4, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyi commented Mar 4, 2019 • edited Loading

jwalgran commented Mar 4, 2019 • edited Loading

kellyi commented Mar 4, 2019

kellyi commented Mar 5, 2019

kellyi commented Mar 5, 2019

kellyi commented Mar 5, 2019

kellyi commented Mar 5, 2019

jwalgran commented Mar 5, 2019

jwalgran left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kellyi commented Mar 5, 2019

kellyi commented Mar 5, 2019

kellyi commented Feb 28, 2019 •

edited

Loading

kellyi commented Mar 4, 2019 •

edited

Loading

jwalgran commented Mar 4, 2019 •

edited

Loading