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

feat(budget): list page budget summaries #215

Merged
merged 15 commits into from
Jan 25, 2024
Merged

Conversation

OchiengPaul442
Copy link
Collaborator

@OchiengPaul442 OchiengPaul442 commented Jan 16, 2024

Description

  1. Implemented a new "Budget Summary" component to efficiently manage iterations and capture comprehensive details for each saved budget.
  2. Refined the UI of the saved budget card to display summary details in a visually appealing and less cluttered manner.
  3. Incorporated tooltips to provide full descriptions.
  4. Incorporated icons for various summary items on the card, utilizing assets saved in the assets folder.
  5. Added placeholder images, which can be updated later with designs from the design team, for the following:
    • total family labour
    • total input value
    • total output value
    • final cash balance

Discussion

This PR addresses and resolves Issue #205.

Preview

Link to app preview if relevant

Screenshots / Videos

image

@OchiengPaul442 OchiengPaul442 self-assigned this Jan 16, 2024
@OchiengPaul442 OchiengPaul442 linked an issue Jan 16, 2024 that may be closed by this pull request
Copy link
Collaborator

@chrismclarke chrismclarke left a 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;
Copy link
Collaborator

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;
Copy link
Collaborator

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;
Copy link
Collaborator

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

this.calculateSummary();
}

calculateSummary() {
Copy link
Collaborator

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

@OchiengPaul442
Copy link
Collaborator Author

Thank you, @chrismclarke, for your feedback. I will address the highlighted issues.

@chrismclarke chrismclarke self-requested a review January 25, 2024 20:25
Copy link
Collaborator

@chrismclarke chrismclarke left a 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

localhost_4200_budget(PICSA-Tablet)

But overall I think really nice to have so thanks again for all the effort put in.

@chrismclarke chrismclarke changed the title Detailed summary for saved budgets feat(budget): list page budget summaries Jan 25, 2024
@chrismclarke chrismclarke added the Tool: Budget Updates related to Budget tool label Jan 25, 2024
@chrismclarke chrismclarke merged commit 1a6a76a into main Jan 25, 2024
5 checks passed
@chrismclarke chrismclarke deleted the ft-budget-detailed-summary branch January 25, 2024 20:48
@github-actions github-actions bot added feature and removed Tool: Budget Updates related to Budget tool labels Jan 25, 2024
@chrismclarke chrismclarke added the Tool: Budget Updates related to Budget tool label Feb 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready for review Tool: Budget Updates related to Budget tool
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

feat(budget): detailed summary
2 participants