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: upcoming payments grid #159

Merged

Conversation

abGit9
Copy link
Contributor

@abGit9 abGit9 commented Mar 4, 2024

This pull request makes the following changes:

Addresses issue #117
Is a follow up to PR #156

Adds feature where upcoming payments can be viewed in a grid. Users can toggle between the default row format and grid format.

@DennisBauer
Copy link
Owner

Thank you for the PR!

Copy link
Owner

Choose a reason for hiding this comment

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

I really like that you extracted the dimensions used in both of the layouts.
Just a few suggestions: Please move the dimension into another file called dimens.xml to separate it from the pure integer values. Also recheck the usage of the dimensions, some of them are not only used for the grid but also the default layout. Therefore I could think of the following naming scheme:

<dimen name="overview_???">...</dimen>
<dimen name="overview_grid_???">...</dimen>
<dimen name="overview_large_???">...</dimen>

But I'm also open for other suggestions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, will do.

@abGit9 abGit9 force-pushed the feature_expenses_grid_upcoming_payments branch from b7a24c5 to 8ab9fa2 Compare March 4, 2024 23:58
@abGit9
Copy link
Contributor Author

abGit9 commented Mar 5, 2024

There is more to be done. I think that we need to style the recurring expense and upcoming payment items. Also, might want to consider persistence options for saving scrolling location of each list/grid. Just a suggestion, but I think it would be good to modularize the pull requests and issues as much as possible. It makes it easier to track different features, bugs, design ideas etc.

@abGit9
Copy link
Contributor Author

abGit9 commented Mar 5, 2024

Appreciate the collaboration. Thanks.

@DennisBauer
Copy link
Owner

There is more to be done. I think that we need to style the recurring expense and upcoming payment items. Also, might want to consider persistence options for saving scrolling location of each list/grid. Just a suggestion, but I think it would be good to modularize the pull requests and issues as much as possible. It makes it easier to track different features, bugs, design ideas etc.

I like your approach. I realized the issue with the scroll position when checking your first PR. Removing the animation resolves this automatically keeping the scroll position for both grid and normal layout, however I haven't looked further into it.

@DennisBauer
Copy link
Owner

Approve your PR as is, do you want to squash your refactor commit before I merge it?

@abGit9
Copy link
Contributor Author

abGit9 commented Mar 6, 2024

I like your approach. I realized the issue with the scroll position when checking your first PR. Removing the animation resolves this automatically keeping the scroll position for both grid and normal layout, however I haven't looked further into it.

Thanks. Yes, you certainly did bring it up! We can work out the issue with scroll position later on. Either keep the animation and find a work around or make a decision to prioritize the scroll position and remove the animations. I am open to both. Let's look into it more and discuss it in the future.

@abGit9
Copy link
Contributor Author

abGit9 commented Mar 6, 2024

Approve your PR as is, do you want to squash your refactor commit before I merge it?

I think we can keep the refactor commit if that is ok? There is 1 refactor commit and it applies the modifications(dimens etc) that you suggested. Otherwise, I'm good to go on this PR. I'll continue to work on issues we've mentioned.

@DennisBauer DennisBauer merged commit 0ef73c1 into DennisBauer:main Mar 6, 2024
2 checks passed
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.

2 participants