-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat(budget): list page budget summaries #215
Conversation
…budget-detailed-summary
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.
Thanks @OchiengPaul442
This is definitely headed in the right direction. I appreciate all the different calculations you have made and also really like the use of mat-tooltips to provide extra context. The code is also organised in a nice and consise way which is great.
As the app will be used by farmers across multiple countries/languages it would be good if we could try to use more visuals instead of abbreviations for the summaries. E.g. using the family member images next to the total number of family hours (instead of TFLH
acronym)
I don't think we currently have icons to represent inputs, outputs or cash balance, so I would suggest just using any sort of placeholder icon and we can get some designed.
As for produce consumed, would it be possible to show the summary by crop, e.g. if 2 quantities of maize consumed and 1 of wood show icons for both maize and wood alongside the total numbers?
Otherwise I've added a few minor (non-blocking) comments inline (using the word nit
from the english "nit-pick", basically just meaning minor faults, in case you haven't come across that word before).
display: flex; | ||
flex-direction: column; | ||
justify-content: flex-start; | ||
margin-bottom: 1.2rem; |
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.
nit(non-blocking)
It would probably be better if the component doesn't define its own margin, and instead allows the parent to do so.
To achieve this you should add a css style for:
host:{
display: block
}
then when the component is used it should respond as expected to styling, e.g.
<budget-summary style=margin-bottom:1.2rem />
margin: 0 0 0.5rem 0; | ||
padding: 0; | ||
font-size: 1rem; | ||
color: #000; |
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.
nit(non-blocking)
You can probably just use the inherited color and remove this css.
Also if you want to use a h2 element you should leave the font size for consistency across the app. If the font size is too big consider a h3 or h4 element instead, or just using a div with font-weight bold
p { | ||
margin: 0; | ||
padding: 0; | ||
font-size: 1rem; |
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.
nit(non-blocking)
likely here the font-size
and color
styles are the same as inherited and not required
apps/picsa-tools/budget-tool/src/app/components/budget-summary/budget-summary.component.ts
Outdated
Show resolved
Hide resolved
this.calculateSummary(); | ||
} | ||
|
||
calculateSummary() { |
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 concise methods to calculate the totals
Thank you, @chrismclarke, for your feedback. I will address the highlighted issues. |
…budget-detailed-summary
remove legacy registered icons and add code to register budget icons to mat-icon registry
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.
Many thanks for making all the updates @OchiengPaul442
I noticed a couple more minor issues with I've added commits to update, but just for your interest:
Inline function calls
You used methods like getImagePath('cash-balance', 'svg')
to load assets, which is handy as the crop images are still a mix of svg/png files so a bit of a pain to load just based on name (future TODO issue). The downside to calling in the template html code is it will get retriggered every render, which can be a little inefficient. Instead I've called the function in the ts code just when new items are found
Icons
Thanks for adding some extra icons. As mentioned above, the way icons in the budget tool are handled are a bit messy (mix of svg/png and lots of on-demand loading).
A nicer method is to register the svgs with the MatIconRegistry, and then you can just use inline via the name, e.g. <mat-icon svgIcon='cash-balance'>
, I've updated the docs to outline the method here: https://docs.picsa.app/advanced/icons
I've also moved the new svgs to a standalone folder and updated the material module to register them via dcfbbf2
Type definitions
There's quite a few places you used any
as a type. Usually this isn't ideal as can lead to more errors in the code, which can be problematic in this codebase particularly as there aren't many unit tests integrated. So where possible using strong type definitions is always preferred (you can see the types I added in 700e144
UI updates
I really liked your updated UI. I've made a few minor changes to make the summary sit to the right of the budget name, and removed (for now) input/output individual values to keep more slim (although only commented out for now as will check with the University team if they would prefer to keep or not). I was also able to reduce the amount of css required a little, screenshot below
But overall I think really nice to have so thanks again for all the effort put in.
Description
Discussion
This PR addresses and resolves Issue #205.
Preview
Link to app preview if relevant
Screenshots / Videos