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

feature/matter-page #222

Merged
merged 4 commits into from Jun 7, 2022
Merged

Conversation

BrianL3
Copy link
Collaborator

@BrianL3 BrianL3 commented May 12, 2022

Link to Relevant Issue

This pull request resolves #56

Description of Changes

This creates the matters/:id route and corresponding page. Everywhere a matter is referenced or discussed (a Person's legislation history, a meeting votes table, an event page) there should now be a link to the associated matter.

Link to Forked Storybook Site

The site

@BrianL3 BrianL3 changed the title Feature/matter page feature/matter-page May 12, 2022
@evamaxfield evamaxfield added the enhancement New feature or request label May 12, 2022
@evamaxfield
Copy link
Member

Wooooooo! So excited and already loving having this available even in brians fork 😂

I got some comments and I haven't looked at the code yet so if there are answers in the code that I should be aware of, just ignore me:

  1. How are sponsors of matters determined? For the ones I have checked there is only ever 1 sponsor but I know that matters can have many sponsors.
  2. Nitpick maybe, but why have multiple status icons?
    image
  3. When I expand the latest vote, there should be details about "Click to expand" to see full voting record. Not sure what text should go there but something should inform the user that they can see the full vote details.
  4. In the "history" part, can we place the meeting date and the body as the first text:
    image

I.e.

* "Full Council -- May 10, 2022"
  "Votes >"
  "Documents >"
* "Transportation Committee -- May 1, 2022"
  "Votes >"
  "Documents >"
  1. Similarly, because we aren't currently storing what the vote was for (tabling, passage to full council, passage of full council, etc.) I actually think we may want to remove the "Adopted" / markers from each history point.
  2. Similarly, the votes in the history should have some text saying "expand" or something
  3. Question for all of us.... are we duplicating data / did we get the logic of minutes item documents and matter documents wrong on the backend or is it frontend?
    image

Two different events have the same attachments.

@evamaxfield
Copy link
Member

Also, I think cc @NeelApte if you want to follow review process

Copy link
Collaborator

@tohuynh tohuynh left a comment

Choose a reason for hiding this comment

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

Thanks for getting this done. Overall, it looks great.

I have a few asks that I think would improve the page a bit.

src/components/Details/Legislation/LegislationOverview.tsx Outdated Show resolved Hide resolved
src/containers/MatterContainer/MatterContainer.tsx Outdated Show resolved Hide resolved
src/containers/PersonContainer/MattersSponsored.tsx Outdated Show resolved Hide resolved
src/networking/MatterStatusService.ts Outdated Show resolved Hide resolved
Comment on lines 41 to 50
const matterSponsors = await matterSponsorService.getMatterSponsorByMatterId(id);
const sponsors = matterSponsors
.filter((matterSponsor) => {
return matterSponsor.person;
})
.map((matterSponsor) => {
return matterSponsor.person as Person;
});

const matterStatuses = await matterStatusService.getMatterStatusesByMatterId(id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could we group lines 41 and 50 into one Promise.all so that these two pieces of data are fetched concurrently. It will speed up the load of the page by a little.

src/pages/MatterPage/MatterPage.tsx Outdated Show resolved Hide resolved
src/pages/MatterPage/MatterPage.tsx Outdated Show resolved Hide resolved
@tohuynh tohuynh linked an issue May 13, 2022 that may be closed by this pull request
@tohuynh
Copy link
Collaborator

tohuynh commented May 13, 2022

Nitpick maybe, but why have multiple status icons?

It was in the wireframes. I'm OK with removing one of the status icons. But if we add vertical spacing between each sub components, it would help?

Similarly, the votes in the history should have some text saying "expand" or something

I think the triangle next to Votes is a common UI pattern used to indicate something is an accordion -- that is there are some more details if clicked on. Though the inconsistency between the triangle next to Votes and the + sign next to See Documents sucks. Maybe make them both have triangles or +s?

Question for all of us.... are we duplicating data / did we get the logic of minutes item documents and matter documents wrong on the backend or is it frontend?

I think it could be backend. The list of files fetched here, just fetch for the files by the event_minutes_item.id. It could also be that the two matter statuses' event_minutes_item_ref point to the same event_minutes_item. Or that the two matter status are the same.

@evamaxfield
Copy link
Member

I think the triangle next to Votes is a common UI pattern used to indicate something is an accordion -- that is there are some more details if clicked on. Though the inconsistency between the triangle next to Votes and the + sign next to See Documents sucks. Maybe make them both have triangles or +s?

This is good in regards to consistency between the two but I am talking about expanding to each voting bar to reveal the individual votes for that "bar".

I think it could be backend. The list of files fetched here, just fetch for the files by the event_minutes_item.id. It could also be that the two matter statuses' event_minutes_item_ref point to the same event_minutes_item. Or that the two matter status are the same.

I will have to think on this a bunch and figure out what is going wrong.

@tohuynh
Copy link
Collaborator

tohuynh commented May 13, 2022

I am talking about expanding to each voting bar to reveal the individual votes for that "bar".

Oh, I see. I agree then. Let's save that for another PR to improve the LatestVote and LegislationHistory components.

@tohuynh tohuynh self-requested a review June 7, 2022 20:27
Copy link
Collaborator

@tohuynh tohuynh left a comment

Choose a reason for hiding this comment

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

Thanks for making the changes. Looks good to me, merge whenever.

@BrianL3 BrianL3 merged commit 7ee8245 into CouncilDataProject:main Jun 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[f_legislationtracking] Design & Web App: Legislation Page
3 participants