-
Notifications
You must be signed in to change notification settings - Fork 1
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
Define interface for selecting Census regions #222
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking great. I'm not actually able to create new Philadelphia census areas—did you intend that temporary breakage? In terms of user flow, though, I think this works!
I have two thoughts on storing default region
values: I think we could either add a default_region
field to the [django-pldp Agency model](https://github.com/datamade/django-pldp/blob/master/pldp/models/agency.py or to the custom User model in this repo. The advantage of attaching it to user is that it would accommodate theoretical multi-regional agencies; the advantage of attaching it to Agency is that region wouldn't have to be set for each individual user. What do you think?
model = survey_models.CensusArea | ||
fields = ['name', 'agency', 'region'] | ||
|
||
def add_inputs(self, helper): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah nice!
I did a little bit of debugging and was able to figure out the root problem with creating new CensusAreas, which I've captured in #223. Tomorrow I'll add in a fix for it, add |
@beamalsky Pushed up a couple of changes -- one commit fixes #223, and the other adds a loading spinner via a modal to give more of a hint that a new page is loading (not a comment you left, just something I noticed as I was testing it out). I decided not to implement |
@beamalsky It should be below the map! Do you see it there? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your handling of the agency checkbox makes a lot of sense. LGTM! Holding off on a default_region
attribute until we know if it'll be useful sounds like the right move.
Overview
This PR sketches out the UI for selecting Census regions when creating new CensusAreas. Also, fix a bug where
View.get_queryset_for_agency()
improperly caches the queryset.Connects #221, closes #223.
Notes
I wanted to get this PR in before actually adding new CensusRegions so that we could figure out what the interface should look like. Because of this, the new view won't really show any new regions -- it should simply illustrate what the user flow will look like, with Philadelphia as the only currently-supported region.
Allow user to create CensusAreas for new cities #221 suggests that we might try to create a "shortcut" workflow that will skip the region selection step for power users who only create CensusAreas in one region. I didn't add that as part of this PR yet but I'd be curious to hear if you have thoughts about how we might achieve that -- there doesn't seem to me to be anywhere obvious where it would make sense.
Testing Instructions
Create new census area
buttonPhiladelphia
from the Region selector