-
-
Notifications
You must be signed in to change notification settings - Fork 937
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
[Mobile] Budget table revamp #2642
Conversation
✅ Deploy Preview for actualbudget ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
Bundle Stats — desktop-clientHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset
View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger
Smaller No assets were smaller Unchanged
|
Bundle Stats — loot-coreHey there, this message comes from a GitHub action that helps you and reviewers to understand how these changes affect the size of this project's bundle. As this PR is updated, I'll keep you updated on how the bundle size is impacted. Total
Changeset No files were changed View detailed bundle breakdownAdded No assets were added Removed No assets were removed Bigger No assets were bigger Smaller No assets were smaller Unchanged
|
dead25a
to
43a7a6f
Compare
e0d1ea1
to
f4e88d2
Compare
4ea0e0d
to
4ffe0e3
Compare
@jsehnoutka Thanks for the feedback! Pushed some changes to resize text when amount gets too long |
@joel-jeremy Looks great now, thanks for your work! Would it be possible to leave the same amount of characters available for the category name though? Maybe put the new > UI element a little bit to the right and assign fixed position to it? There are now less characters available for the category name before the line breaks, also the > UI element does not look very streamlined because it shows on different positions depending on the category name length. I think it would look better if it had consistent place on each line (category), nevertheless the text lenght. Before: After: |
Agree, category chevron would look best aligned with the group chevron. Not too sure about it changing alignment between groups, but when aligned with the group one you'll know the position will always work for the categories within the group as the group level totals the values underneath so will always have the widest numbers. |
Pushed an update
Yes it's tapable |
I not sure what I think about the lined up category arrows. Im fine with all the other changes though |
I'm also torn on lining up the cheverons on the group/category names. Lining them up looks good visually but also kinda wastes some space. I'm a leaning a bit towards not lining them up since that's how the desktop does it in the desktop budget table. More feedback would be good here. |
cca2579
to
185bc42
Compare
@MatissJanis Any pending @psybers @youngcw @Teprifer @jsehnoutka Any pending feedback on this? |
@joel-jeremy Although it looks nice, having the chevrons lined up on the right makes it less obvious they are connected to the text to the left of them and thus it is much less obvious that the whole thing is one giant link. For example, here: It is not obvious what clicking on the text will do. Since it is closer to the triangle, it feels like it would trigger that effect and not the chevron's effect. But in terms of the code, most looks fine. It is hard to mentally parse the diff for the budget table since it is so long, but I can't spot any obvious issues. |
Tested on iPhone 14: worked as expected. And the design is much better than before. I'm very happy with the changes you've done here and at this point I'd recommend merging it in ASAP (after v24.6.0 release). We can always iterate on the design afterwards. As for code review: let me know if you'd want me to do it. Happy to help with that too, but you might have to wait a while longer for my review (my current started-and-not-finished review queue is quite long already, so I'm trying to get through those first before picking up new things..). So if someone reviews and approves in the meantime - that's ok too :) |
I think this is a huge step forward regarding design. Great job! And thanks for adressing my concern with the available category text lenght :) |
@MatissJanis @psybers I think this is ready for another round of review and/or get merged to the edge so we can collect the most feedback until next release. |
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.
LGTM. This is quite a big change, so has potential to break other things. However, we can always fix things in follow-up PRs. Don't want to block this for any longer.
No description provided.