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

Dev-4347 account spending tables #1706

Merged
merged 26 commits into from
Jun 2, 2020
Merged

Conversation

jameslee40
Copy link
Contributor

@jameslee40 jameslee40 commented May 29, 2020

High level description:

Adds account spending table to the agency v2 profile page.

Technical details:

  • Added / Updated account spending TableContainer with a parseAccount function, and loading / error states.
  • Added new data model BaseAccountSpendingRow and agency data mapping tableColumns.
  • Updated CountTabContainer and CountTab to include disabled prop when count === 0.

JIRA Ticket:
DEV-4347

Testing:
URL should use a FREC or CGAC code instead of the agency database id.
i.e. https://www.usaspending.gov/#/agency_v2/020

Mockup:
N/A

The following are ALL required for the PR to be merged:

Author:

Reviewer(s):

  • Design review complete if applicable
  • [N/A] API #1234 merged concurrently if applicable
  • Code review complete

@@ -66,6 +66,11 @@
line-height: rem(20);
}
}

&:disabled {
color: #737373;
Copy link
Contributor

Choose a reason for hiding this comment

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

looks a lot like our color variable $color-gray-medium in core/_variables.scss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.


&:disabled {
color: #737373;
background-color: #FAFAFA;
Copy link
Contributor

Choose a reason for hiding this comment

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

looks a lot like our color variable $color-gray-lightest in core/_variables.scss

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated.

};

const CountTab = (props) => (
<button
className={`count-tabs__button${props.active ? ' count-tabs__button_active' : ''}`}
onClick={() => props.setActiveTab(props.type)}>
onClick={() => props.setActiveTab(props.type)}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should be avoiding binding and arrow function in render whenever possible from React docs Using an arrow function in render creates a new function each time the component renders, which may break optimizations based on strict identity comparison. ( https://reactjs.org/docs/faq-functions.html ). We can move this function out to be const setActiveTab = (e) => props.setActiveTab(e.target.value) and pass a value prop to the button by value={props.type}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored.

const budgetaryResources = Object.create(BaseAgencyBudgetaryResources);
budgetaryResources.populate(res.data);
// store the data model object in Redux
dispatch(setBudgetaryResources(budgetaryResources));
Copy link
Contributor

Choose a reason for hiding this comment

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

Very Nice!

budgetaryResources.populate(res.data);
// store the data model object in Redux
dispatch(setBudgetaryResources(budgetaryResources));
});
Copy link
Contributor

Choose a reason for hiding this comment

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

.catch() how are we handling errors for this content?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added error handling for this, as well as, rendering the error.


const TableContainer = (props) => {
const [currentPage, changePage] = useState(1);
const [pageSize, changeLimit] = useState(10);
Copy link
Contributor

Choose a reason for hiding this comment

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

changePageSize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

};

const TableContainer = (props) => {
const [currentPage, changePage] = useState(1);
Copy link
Contributor

Choose a reason for hiding this comment

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

changeCurrentPage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated!

const [loading, setLoading] = useState(true);
const [error, setError] = useState(false);
// TODO - use totalObligation to calculate '% of Total Obligations' column value
const totalObligation = useSelector((state) => state.agencyV2.budgetaryResources._agencyTotalObligated);
Copy link
Contributor

Choose a reason for hiding this comment

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

Very Nice!

sort: accountFields[sort],
order
};
const request = fetchSpendingByCategory(props.agencyId, props.type, params);
Copy link
Contributor

Choose a reason for hiding this comment

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

We should migrate this to it's own function with useCallback() since we are doing the same thing in two effects with the only difference setTotalItems(res.data.page_metadata.total); and we can just do that both times or add some logic to see it it has been populated and then call it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a function using useCallback(), but not sure it's doing anything because we're not passing anything in its dependency array. Let me know, if you have any idea how it should work.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking good. Thank you. We cannot pass parameters to useCallback that I am aware of. So we would need to move

        setLoading(true);
        // Make a request with the new page number
        const params = {
            fiscal_year: props.fy,
            limit: pageSize,
            page: currentPage,
            sort: accountFields[sort],
            order
        };

into fetchSpendingByCategoryCallback as well and then it should work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

get percentOfTotalObligations() {
if (this._totalObligatedAmount > 0) {
const percentage = generatePercentage(this._obligatedAmount / this._totalObligatedAmount);
if (percentage === ('0.00%')) return 'Less than 0.01%';
Copy link
Contributor

Choose a reason for hiding this comment

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

curious as to why we have the parens around the ('0.00%')?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No reason, I removed it.

className={`count-tabs__button${props.active ? ' count-tabs__button_active' : ''}`}
onClick={setActiveTab}
disabled={props.disabled}
value={props.type}>
Copy link
Contributor

Choose a reason for hiding this comment

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

We do not need value anymore since we are not dynamically setting the value of this button. That was an oversight on my part.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I removed it. Thanks @jonhill13!

@jameslee40 jameslee40 merged commit f7ed2b8 into dev Jun 2, 2020
@jameslee40 jameslee40 deleted the ftr/dev-4347-account-spending-tables branch June 2, 2020 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants