[WIP] Full grants table #52
Conversation
src/containers/GrantsTableWrapper.js
Outdated
|
|
||
| if (!data.organization) return `Failed to load org data!`; | ||
|
|
||
| if (data.organization.grantsFunded.length < props.countGrantsFrom || data.organization.grantsReceived.length < props.countGrantsTo) { |
There was a problem hiding this comment.
Could this get a short comment explaining what it's for? Or breaking the conditionals out into variables could serve the same function, something like:
const moreGrantsFundedToLoad = data.organization.grantsFunded.length < props.countGrantsFrom
const moreGrantsReceivedToLoad = data.organization.grantsReceived.length < props.countGrantsTo
if (moreGrantsFundedToLoad || moreGrantsReceivedToLoad) {
...There was a problem hiding this comment.
yes, good call - updated 👍
| and ${data.organization.grantsReceived.length}/${props.countGrantsTo} grants received.` | ||
| ); | ||
|
|
||
| const { |
There was a problem hiding this comment.
Moving this block up around line 55 could shorten some of the code above, if I'm reading it correctly
There was a problem hiding this comment.
Ah so this is a bit tricky -- we need the orginal length of the grantsFunded/grantsReceived response pre-cleanse in order to properly set offset. Because the cleanse will create and insert the "org header rows" directly into the array, we'd be trying to fetch something like 182/171 possible grants if this is defined upfront.
Maybe a phase 2 refactor though, would be nice to have shorter variable names here.
src/containers/GrantsTableWrapper.js
Outdated
|
|
||
| const cleanse = (organization) => { | ||
| // Sort lists by org | ||
| const flattenedGrantsReceived = sortBy( |
There was a problem hiding this comment.
This is a total personal preference, but I would advocate splitting this into two steps for clarity. First the map to generate the list, then a second step for the sortBy.
The reason I suggest that is the sortBy call is ~20 lines away from the sort logic, so I was originally confused as to what was happening here. I thought it was super complicated sort logic! Then I realized it's lodash and not Array.prototype.sort()
| const allYears = Object.keys({ | ||
| ...fundedYearlySums, | ||
| ...receivedYearlySums, | ||
| }).reduce((acc, cur) => ({ ...acc, [cur]: 0 }), {}); |
There was a problem hiding this comment.
could this get longer variable names?
| ); | ||
| }; | ||
|
|
||
| const cleanse = (organization) => { |
There was a problem hiding this comment.
Could this get a short comment about its role?
Fetching all grantsFunded and grantsReceived for an Organization so we render accurate totals in the yearly bar charts and show all grants in the table.
Main updates:
grantsFunded&grantsReceivedquery to its own containerGrantsTableWrapper. Even though fetchMore can execute it's own query, I'm figuring a separate container gives us a much nicer loading experience for orgs with a lot of grants (see CFSEM for example)Minor styling tweaks:
Future improvements outlined here #55