Skip to content
This repository has been archived by the owner on Feb 1, 2024. It is now read-only.

Wrap facility search results with react-infinite via react-infininte-any-height #711

Merged
merged 6 commits into from Jul 31, 2019

Conversation

jwalgran
Copy link
Contributor

@jwalgran jwalgran commented Jul 29, 2019

Overview

Wraps search facility results with react-infinite via react-infininte-any-height in preparation for loading more than 500 results.

Connects #705

Demo

2019-07-30 10 18 17

Notes

React infinite requires that we specify the height of the list item container. We accomplish this by listening for window resize events, tracking the window size in the Redux state, then calculating the proper container size.

react-infinite-any-height is a wrapper around react-infinite that wraps each list item with a span and uses that span to calculate the height, preventing the need to force all list items to be the same height. There is a lot of variability in the length of facility names and addresses. On narrow screens some list items can be twice as tall as others.

Testing Instructions

  • Run ./scripts/update to install the new packages
  • Verify that the facility search UI works as before

Checklist

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

There was a single dependency in the `package.json` file that was not in correct
alphabetical order.
We will use this to load additional facility search results on demand. Added by
running this command on the development Vagrant VM.

```
docker-compose \
  run --rm --no-deps app \
  yarn add react-infinite@0.13.0
```
@jwalgran jwalgran changed the title Wrap facility search results with react-infinite Wrap facility search results with react-infinite via react-infininte-any-height Jul 30, 2019
@jwalgran jwalgran marked this pull request as ready for review July 30, 2019 17:23
@jwalgran jwalgran requested a review from kellyi July 30, 2019 17:23
@@ -25,6 +25,8 @@
"react-copy-to-clipboard": "5.0.1",
"react-dom": "16.8.6",
"react-hot-loader": "4.9.0",
"react-infinite": "0.13.0",
"react-infinite-any-height": "^2.3.0",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider removing ^ here just to stay in tune with the other imports.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Will do.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pinned the react-infinite-any-height version in fixup a9c5d89

138, // Sidebar controls
51, // Footer
8, // Additional padding
];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wonder if we should set these values as constants somewhere if they're hard-coded in multiple places?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea. Since we are styling with JS objects, we should have the luxury of just moving the values out to a shared constant.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Turns out it is not that easy. There are min-height and padding directives coming in from our top level stylesheet as well as Material UI that are complicating things. I am going to instead add class tags to the elements that we need to measure and do the height calculation on the fly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Implemented a class-based height calculation in fixup abdafcf

@@ -20,6 +21,10 @@ const initialState = Object.freeze({
filterText: '',
resetButtonClickCount: 0,
}),
window: Object.freeze({
innerHeight: typeof window === 'object' ? window.innerHeight : null,
innerWidth: typeof window === 'object' ? window.innerWidth : null,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also https://lodash.com/docs/4.17.15#isObject but it probably does the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interestingly there is a subtle difference. _.isObject considers a function to be an "object"

https://github.com/lodash/lodash/blob/9f865886f8ae22b205107b00b7b65871bfc9a2f0/lodash.isobject/index.js#L35-L38

Good to know, but it doesn't matter in this case. Switching to isObject will make things a bit more readable.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Switched to using isObject in fixup 2952bdf

@kellyi
Copy link
Contributor

kellyi commented Jul 30, 2019

We may not need to worry about them but it appears that react-infinite & react-infinite-any-height cause some peer dependency warnings on yarn install.

Copy link
Contributor

@kellyi kellyi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 This works well!

Screen Shot 2019-07-30 at 4 59 56 PM

I noticed that the CHANGELOG wasn't updated with this PR, so reminder to update that before merging.

@kellyi kellyi assigned jwalgran and unassigned kellyi Jul 30, 2019
Tracking the window size will allow components with fixes sizes to respond
accordingly. For example, `react-infinite` requires us to specify a fixed
container height.
We plan to allow for the display of all matching search results rather than
capping the result count at 500. Using `react-infinite` will allow us to trigger
a request for additional items when the person searching for facilities scrolls
to the bottom of the partial list.

`react-infinite` forces us to explicitly set the size of the container and the
size of each item. We are temporarily using a hardcoded 120px height for list
items which looks ok for most results. In a future commit we can develop a
"height calculator" that would allow us to resume using variable height list
items.
`react-infinite-any-height` is a wrapper around `react-infinite` that wraps each
list item with a span and uses that span to calculate the height, preventing the
need to force all list items to be the same height. There is a lot of
variability in the length of facility names and addresses. On narrow screens
some list items can be twice as tall as others.
@jwalgran
Copy link
Contributor Author

Thanks for the excellent suggestions.

@jwalgran jwalgran merged commit f8b970d into develop Jul 31, 2019
@jwalgran jwalgran deleted the jcw/infinite branch July 31, 2019 19:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants