component/people-page #209

BrianL3 · 2022-03-18T19:43:49Z

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.

tohuynh

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.

src/components/Cards/PersonCard/PersonCard.tsx

tohuynh · 2022-03-18T22:55:25Z

src/networking/RoleService.ts

+          doc(NetworkService.getDb(), COLLECTION_NAME.Seat, seatId)
+        ),
+        limit(1),
+        orderBy("end_datetime", ORDER_DIRECTION.desc),


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?

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?

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)

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?

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?

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?

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.

src/networking/RoleService.ts

src/containers/PeopleContainer/PeopleContainer.tsx

BrianL3 · 2022-03-19T00:33:05Z

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).

Did this.

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.

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.

tohuynh · 2022-03-19T01:00:02Z

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

src/networking/SeatService.ts

evamaxfield

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.

BrianL3 · 2022-03-21T19:12:33Z

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

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.

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.

I will put that and search stuff into a different PR that concentrates more on adding search.

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.

I may need you to make another index for this, I am not sure. I put an orderBy seat name, that should hopefully do it.

evamaxfield · 2022-03-21T21:11:18Z

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.

I may need you to make another index for this, I am not sure. I put an orderBy seat name, that should hopefully do it.

Cool. We can do that in a later PR too. No worries for this one.

…atetime roles

tohuynh

Looks good! Thanks for making the changes.

I have maybe one tiny improvement.

tohuynh · 2022-03-25T01:35:03Z

src/pages/PeoplePage/PeoplePage.tsx

+    // 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);
+    }


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

evamaxfield · 2022-03-25T17:22:49Z

@BrianL3 seattle-staging index change now deployed. The new page looks great! Feel free to merge whenever.

PersonCards to be used on the PeoplePage.

b63a250

BrianL3 requested a review from tohuynh March 18, 2022 19:43

tohuynh reviewed Mar 18, 2022

View reviewed changes

PR feedback

cae5a08

tohuynh reviewed Mar 19, 2022

View reviewed changes

src/networking/SeatService.ts Outdated Show resolved Hide resolved

evamaxfield approved these changes Mar 20, 2022

View reviewed changes

Brian Ledbetter added 2 commits March 24, 2022 17:55

Order by Seat name

5229198

changed the query for most recent role for a seat to favor null end_d…

4172b33

…atetime roles

tohuynh approved these changes Mar 25, 2022

View reviewed changes

BrianL3 merged commit e8e7508 into CouncilDataProject:main Apr 1, 2022

tohuynh mentioned this pull request Apr 6, 2022

fix/use-placeholderwrapper-in-person-card #216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

component/people-page #209

component/people-page #209

BrianL3 commented Mar 18, 2022

tohuynh left a comment

tohuynh Mar 18, 2022

BrianL3 Mar 18, 2022

tohuynh Mar 18, 2022

BrianL3 Mar 19, 2022

tohuynh Mar 19, 2022

evamaxfield Mar 20, 2022

BrianL3 Mar 21, 2022

BrianL3 commented Mar 19, 2022

tohuynh commented Mar 19, 2022

evamaxfield left a comment

BrianL3 commented Mar 21, 2022 •

edited

evamaxfield commented Mar 21, 2022

tohuynh left a comment •

edited

tohuynh Mar 25, 2022

evamaxfield commented Mar 25, 2022

component/people-page #209

component/people-page #209

Conversation

BrianL3 commented Mar 18, 2022

Link to Relevant Issue

Description of Changes

Link to Deployed Staged Environment

tohuynh 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

BrianL3 commented Mar 19, 2022

tohuynh commented Mar 19, 2022

evamaxfield left a comment

Choose a reason for hiding this comment

BrianL3 commented Mar 21, 2022 • edited

evamaxfield commented Mar 21, 2022

tohuynh left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evamaxfield commented Mar 25, 2022

BrianL3 commented Mar 21, 2022 •

edited

tohuynh left a comment •

edited