-
-
Notifications
You must be signed in to change notification settings - Fork 106
Add month headers to expenses list #468
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
base: main
Are you sure you want to change the base?
Conversation
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.
I like this idea, but I need to sit on it for a while since this is a significant UI change in the most frequently used part of the app. Currently, I think it's too intrusive and I would make it more minimal, but don't have the bandwidth rn for desig thinking, so this PR might have to wait a bit. Apologies
| return ( | ||
| <> | ||
| {expenses.map((e) => { | ||
| {expenses.reduce((acc, e, idx, arr) => { |
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.
You can call reduce with type params like reduce<JSX.Element[]> and don't make the linter upset with using as :)
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.
with that being set, I believe a map is more semantically aligned with JSX operations like this.
You can simply return a react fragment with an optional header instead
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.
Yeah, just changed to reduce to be able to compare with the previous item easily, in order to detect if the item is the first one in the month
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.
I like this idea, but I need to sit on it for a while since this is a significant UI change in the most frequently used part of the app. Currently, I think it's too intrusive and I would make it more minimal, but don't have the bandwidth rn for desig thinking, so this PR might have to wait a bit. Apologies
Of course, that sounds totally fair. And I only added the header without even styling it :) .
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.
Right, fair point. But I think keeping a let variable outside would be preferred.
I agree with this, scanning through the expenses should stay easy and as uncluttered as possible. However I think having months headers also helps. Some design principles can help adding this without drawing too much attention to it, like :
I can take some time soon to make an example if this helps. Maybe also try to dive in the code and try to design the changes building on your PR if you prefer @rodrigost23 ? |
c59220f to
acacb49
Compare
|
@FelixDz Thanks for your ideas. I just updated the PR addressing @krokosik's concern about using I'm not sure if using a separator and/or centering the text is any better, though:
Anyway, I just feel like not having some sort of separation between months or years, just makes it very hard to find expenses by skimming. |
|
Very nice! Both versions are highly usable in my opinion, I don't have a preference, do you? And what do you think @krokosik ? Also, maybe the padding around the months headers can be reduced a bit, since the display space here is quite critical as @krokosik stated? Maybe try to almost preserve the original spacing between expenses and insert the headers "in between"? Unless you find this spacing important to clearly separate months? Sorry again to be of no help on the code side of this, I didn't have time to solve my dev env setup issues. |








Description
This PR adds month headers to expenses list. I was having trouble going through a long list of expenses, hope this would make it a little more neat.
Demo
Checklist
CONTRIBUTING.mdin its entirety