-
Notifications
You must be signed in to change notification settings - Fork 27
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
component/people-page #209
component/people-page #209
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.
Nice work!
I have two asks -- one straightforward, and the other a little complicated.
Could you use CardsContainer
to layout the person card in PeopleContainer
? See EventsContainer
for an example of how to use CardsContainer
. Using CardsContainer
will layout the cards similar to the other pages and wraps an a
around the card (which makes it tabable and have a cursor).
I like delegating fetching the pictures to PersonCard
. But FetchDataContainer
is not appropriate for this component. When the images are loading, there is a bunch of loading spinners all in the same spot and as a result, it doesn't look like a spinner anymore. The better alternative is to use PlaceholderWrapper
(similar to the cover image in the person page). It's a little complicated to implement and I can make a PR for it.
doc(NetworkService.getDb(), COLLECTION_NAME.Seat, seatId) | ||
), | ||
limit(1), | ||
orderBy("end_datetime", ORDER_DIRECTION.desc), |
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.
If role.end_datetime
happens to be null (which means this is a current role), it will be the role that is returned by this method?
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.
I don't think role.end_datetime
is ever null. If you sort by that property in seattle-staging, there are no documents with a null end_datetime. The current roles now have real dates.
Sorting by a property should return documents that have that property as null. I assume that they are at the end of a list sorted in descending order, though. So, if there ARE null end_datetime
roles, they would be at the end of this list which may be a problem. @JacksonMaxfield care to weigh in?
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.
Yeah, I haven't see a null end_datetime yet, but it's a possibility according to https://github.com/CouncilDataProject/cdp-backend/blob/main/cdp_backend/database/models.py#L199 (not a required field)
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.
I think this is currently the most accurate query we have to get the full council. Maybe we correct if its starts returning accurate results? When did the end_datetime
get populated, if it used to be null
?
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.
When did the end_datetime get populated, if it used to be null?
I think for all the instances we have, end_datetime always had a date.
I think Jackson allowed for "null" end_datetime because there is a possibility of an ongoing role with no end_datetime?
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.
I think for all the instances we have, end_datetime always had a date.
I think Jackson allowed for "null" end_datetime because there is a possibility of an ongoing role with no end_datetime?
Yep. This is true. It may suck but is it possible to do two queries and set-addition the returns?
One query for most recent role by end datetime (like you currently have) and one query for roles with no end_datetime and then set addition them?
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.
My only concern is that the queries for roles with null end_datetime
does leave us in a slightly uncertain state: we are inferring that those null roles found are the most recent, but we are not certain.
I do like the set-addition query better than our currently-deployed solution though, as it is based off of seats (so we won't NO SPONSOR entities). I will implement it.
Did this.
I looked at the PlaceholderWrapper thing and was a bit confused, you can PR it if you want and I can learn how it's done. |
Will do |
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 looks good to me. Approving for now because its already a massive improvement and looks GREAT.
Some extra comments / nitpicks:
- can we make it a three-wide card layout instead of two? Both to match the events page and because the cards look REALLY big on my screen haha
- the page linked only shows the current councilmembers, can we get an "all councilmembers" list / table below or something? Maybe make another issue for that? Iirc, the full councilmembers table / list / w.e. was what Dale was working on and had a design for. We had some cool filter ideas. But that may be a larger issue.
- is it possible to order the councilmembers by name or actually imo by seat name? Like it would be cool if they were always in order of "Position 1", "Position 2", "Position 3", etc.
Yes, I changed the component to re-used the Cards thing from Events. I didn't know that component existed, I am quite pleased to use the generalized components.
I will put that and search stuff into a different PR that concentrates more on adding search.
I may need you to make another index for this, I am not sure. I put an |
Cool. We can do that in a later PR too. No worries for this one. |
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.
Looks good! Thanks for making the changes.
I have maybe one tiny improvement.
// there may be instances where a seat has no Roles, thereby causing errors since Councilors should have Seats | ||
const badRoles = roles.reduce((filtered, optional, index) => { | ||
if (optional === null) { | ||
// there were not ANY roles for this seat, and thus no person to show for it | ||
filtered.push(index); | ||
} | ||
return filtered; | ||
}, [] as number[]); | ||
const goodRoles: Role[] = roles.reduce((filtered, optional) => { | ||
if (optional !== null) { | ||
filtered.push(optional); | ||
} | ||
return filtered; | ||
}, [] as Role[]); | ||
for (let i = badRoles.length - 1; i >= 0; i--) { | ||
// remove the bad, no-Role-having Seats | ||
seats.splice(badRoles[i], 1); | ||
} |
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.
Couldn't this be done in one loop?
roles.reduce((goodRolesAndSeats, role, index) => {
if (role) {
goodRolesAndSeats.roles.push(role)
goodRolesAndSeats.seats.push(seats[index])
}
return goodRolesAndSeats
}, {roles: [], seats: []})
might have to add some typing though
@BrianL3 seattle-staging index change now deployed. The new page looks great! Feel free to merge whenever. |
Link to Relevant Issue
As prep for completion of #164 I added PersonCards to the PeoplePage.
Description of Changes
I also changed the way we query for the current council (those displayed on the People page). This seems to be more accurate and lists the current council without listing any weird entities like "No Sponsor".
PersonCard was refactored as well. Each Card now receives a role(and person) and seat as props, and then fetches its own images.
Link to Deployed Staged Environment
See here to see the new PeoplePage.
Please see
CONTRIBUTING.md
for directions on how this can be done.