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

Add US state button to bottom of agency search results #1028

Merged
merged 3 commits into from
Jun 23, 2017

Conversation

jeremiak
Copy link
Contributor

show-state-search-results

Refs #1009

@jeremiak jeremiak force-pushed the jk-agency-search-info branch from bfb08f4 to 1227aae Compare June 22, 2017 18:03
@brendansudol
Copy link
Contributor

my understanding was that this section would be pinned / always at the bottom of the agency search results box (so you didn't have to scroll to see)

if that's true, do u want to merge this and then have a new PR with that tweak?

@LarryBafundo
Copy link

@brendansudol asking @amberwreed to confirm the plan here.

@LarryBafundo
Copy link

@brendansudol: yep, the button should be pinned/always at the bottom so you dont have to scroll to see it.

@jeremiak
Copy link
Contributor Author

jeremiak commented Jun 23, 2017

@brendansudol I think we should merge this and then try that out in a different PR. I'm concerned that its going to result in a very small amount of scrolling space for a list that is potentially very long.

On the other hand, perhaps it doesn't matter since it has filtering behavior based on what the user enters as an agency name/ORI/etc.

@brendansudol
Copy link
Contributor

brendansudol commented Jun 23, 2017

yep, agreed and we may have to make height of that dropdown a little bigger to accommodate

@@ -37,6 +46,8 @@ const AgencySearchResults = ({ data, groupKey, groupValues, onClick }) => {
data: noFederal.filter(d => d[groupKey] === null),
})

const usState = oriToState(noFederal[0].ori)
Copy link
Contributor

Choose a reason for hiding this comment

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

i think we should pass usState down as a prop rather than parse and lookup from ORIs (since it's available in the LocationFilter component)

Copy link
Contributor

@brendansudol brendansudol left a comment

Choose a reason for hiding this comment

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

one suggested change, otherwise nice!

@brendansudol
Copy link
Contributor

@brendansudol
Copy link
Contributor

🚢

@brendansudol brendansudol merged commit a0a94db into master Jun 23, 2017
@brendansudol brendansudol deleted the jk-agency-search-info branch June 23, 2017 18:31
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

Successfully merging this pull request may close these issues.

3 participants