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

Improve recurrent transactions visibility in the transaction-list page #254

Open
4 tasks done
enrique-lozano opened this issue Nov 14, 2024 · 12 comments
Open
4 tasks done
Assignees
Labels
type: enhancement New feature or request

Comments

@enrique-lozano
Copy link
Owner

enrique-lozano commented Nov 14, 2024

Prerequisites

Current Behavior

With the current app version, we can only see the next item that will come from a recurring transaction, without any extra information. Also, there is no easy way to distinguish future transactions or pending transactions from the rest.

Expected Behavior

It is proposed to do something similar to what the excellent 1Money app is doing right now:

Here we have the should consider the following points:

  • We should introduce a banner with the number of pending transactions that we have
  • The page should be initially scrolled to this banner, hiding everything above it
  • In case of recurrent transactions, only the first value of the recurrence should be displayed, as before
  • All the transactions above this new banner should be greyed-out
  • We have to study if past pending transactions should change from what we have now. That is, apply change can only apply to future pending transactions or to all the pending transactions. I think we should start by applying this to only the future transactions

Steps to Reproduce

To generate a good test for this functionality, it would be good to create at least two recurring transactions, with a small periodicity (weekly or daily) or without an end date. With this, we could obtain a few future transactions and try to filter by dates and other parameters

Additional Information

This issue is based on the discussion generated in #243, with @shinebrillant, @vincius0000 and @enrique-lozano as participants. We should also consider the more in-deep analysis made here.

As always, if you want to collaborate on this, PRs are accepted.

@enrique-lozano enrique-lozano added the type: enhancement New feature or request label Nov 14, 2024
@vincius0000
Copy link
Contributor

I'm excited to work on it, not sure when I will start to, perhaps tomorrow. Shall I comment when I start just to be sure two devs don't work on the same?

@enrique-lozano
Copy link
Owner Author

enrique-lozano commented Nov 15, 2024

Yes please, and assign yourself the task if you want too :)

Thanks a lot!!

@vincius0000
Copy link
Contributor

vincius0000 commented Nov 15, 2024

Last time I've checked I did not have permission to assign myself, must be repository permission settings 🤔

@enrique-lozano
Copy link
Owner Author

Done 😊

@enrique-lozano
Copy link
Owner Author

Hi @vincius0000, please contact me at lozin.technologies@gmail.com if you have any questions or if you want to share your progress

@victor-marino
Copy link

Fully agree with this.

Additionally, I'm thinking perhaps we could offer the option of notifying the user when the deadline of a pending transaction has passed? Otherwise it's easy to forget marking them as paid.

What do you think?

@enrique-lozano
Copy link
Owner Author

Fully agree with this.

Additionally, I'm thinking perhaps we could offer the option of notifying the user when the deadline of a pending transaction has passed? Otherwise it's easy to forget marking them as paid.

What do you think?

Yes, I totally agree. However, we don't have right now any kind of notifications implemented in the app, so it will require a bit of work to introduce them.

That would be part of another task, but for sure a nice addition to the app.

@vincius0000
Copy link
Contributor

vincius0000 commented Feb 6, 2025

@enrique-lozano I've done a very basic change which left me with this

Image

As you can see, future transactions are greyed out.

I'm thinking about refactoring transactions_list.dart file as within the build method we have a ListView.separated(...) method which handles the overall look of the list (inserts first date separator using index == 0, determines if transaction is future transaction then adds opacity, handles separator builders). Now if we wanted to add a text that would say "⬆️ Upcoming transactions ⬆️" in between future and past transactions then this would require crazy logic just to determine if there's any future/past transactions available, figure out at which point to insert this text. We also re-use Capacity widget for every relevant transaction tile in current case which I hate to do.

I figured that separating transactions at stream (using StreamTransformer) and then keeping separate lists would help us here, as then we don't need to complicate existing setup and we'd have to use Opacity widget at most 2 times.

It's one of the bigger changes of course, as to implement this we'll have to re-write the widget and likely use SliverList (to have stable scrolling between lists and any static widget in the middle), the TransactionListComponent widget is used in several places so we'd need to make sure we don't cause any side-effects there.

The screenshot below highlights how different sections (slivers) likely would be organized, the orange line indicates where static widget could be inserted

Image

I have also noticed this snippet of code

if (widget.onTransactionsLoaded != null) {
  widget.onTransactionsLoaded!(allTransactions: transactions);
}

Which doesn't seem to be in use, but if we go with refactoring then schema of the method will likely change (allTransactions would be split into two). I don't know what's the policy here when such methods are changed if this were to be changed, do we gradually deprecate or..?

What are your thoughts on this?
Changed code for reference https://github.com/vincius0000/Monekin/tree/feature/improve-transaction-visibility

@enrique-lozano
Copy link
Owner Author

Hi @vincius0000! Looks good! I honestly think that we will need to create two separate list at some point, for me it's the simpler and easier approach. Probably the hardest part will be to fetch the transactions of the first list (the pending ones, we need to be careful with the DB query) and to make the component scroll initially to the divider.

Regarding the onTransactionsLoaded param, I believe it is used to display the empty list placeholder on some cards. If not it can be removed without a problem. Take into account that as you said, this component is used in several placed.

@vincius0000
Copy link
Contributor

@enrique-lozano should I test existing code more (to see how it deals with different user timezones) and create PR, or let's go with refactoring right away?

@enrique-lozano
Copy link
Owner Author

Probably we can do a mix both, create the PR and let's go with the refactor right away! We can continue discussing code relevant aspects directly in the PR, better than in this thread.

@vincius0000
Copy link
Contributor

Sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants