gavin/maybe
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases 28 Wiki Activity

Add transactions widget to dashboard page #656

Merged
mattia-malnis merged 2 commits from feature/transactions-widget into main 2024-04-23 04:51:06 +08:00
Conversation 13 Commits 2 Files Changed 12 +56 -28
mattia-malnis commented 2024-04-20 03:40:58 +08:00 (Migrated from github.com)
Copy Link

closes #649

I added the transactions widget to the dashboard:

immagine

immagine

@zachgoll for this, I’ve reused the transaction partial. Currently, when a user clicks on a transaction, an edit modal opens. Should we maintain this behavior, or would it be better to make the widget "read-only"?

closes #649 I added the transactions widget to the dashboard: ![immagine](https://github.com/maybe-finance/maybe/assets/15941778/3db71093-ff56-4a18-8d29-daf5f69a5984) ![immagine](https://github.com/maybe-finance/maybe/assets/15941778/c5fc9665-ce73-4e28-81ec-acda29aff255) @zachgoll for this, I’ve reused the [transaction partial](https://github.com/mattia-malnis/maybe/blob/c97ec2cba9e44ce1aae1aa37e2c5444e5983f8fd/app/views/transactions/_transaction.html.erb). Currently, when a user clicks on a transaction, an edit modal opens. Should we maintain this behavior, or would it be better to make the widget "read-only"?
zachgoll commented 2024-04-20 04:48:19 +08:00 (Migrated from github.com)
Copy Link

@mattia-malnis looking good! I think it's okay if we keep the the modal functionality.

@mattia-malnis looking good! I think it's okay if we keep the the modal functionality.
zachgoll (Migrated from github.com) reviewed 2024-04-20 04:51:58 +08:00
app/views/transactions/_transaction.html.erb
@@ -1,17 +1,22 @@
<%# locals: (transaction:) %>
zachgoll (Migrated from github.com) commented 2024-04-20 04:51:58 +08:00
Copy Link

Small nitpick here, but since we're unsure where a transaction will be shown in the UI, do you think instead of is_widget, we introduce something like fields: [:name, :category, :account, :amount] that explicitly tells the partial which transaction fields to display and the sizing automatically works based on that?

Small nitpick here, but since we're unsure _where_ a transaction will be shown in the UI, do you think instead of `is_widget`, we introduce something like `fields: [:name, :category, :account, :amount]` that explicitly tells the partial which transaction fields to display and the sizing automatically works based on that?
mattia-malnis (Migrated from github.com) reviewed 2024-04-20 05:44:55 +08:00
app/views/transactions/_transaction.html.erb
@@ -1,17 +1,22 @@
<%# locals: (transaction:) %>
mattia-malnis (Migrated from github.com) commented 2024-04-20 05:44:55 +08:00
Copy Link

I think introducing the fields concept is a good idea! However, with the current code structure, dynamic size calculation could be tricky. I'm not entirely sure how to handle it because the same partial should fit both a narrow widget and a wide page while keeping the columns aligned.

Using a <table> could potentially fix this issue, but it might be a bit old-fashioned

c46662c99f/app/views/transactions/_transaction.html.erb (L3)
c46662c99f/app/views/transactions/_transaction.html.erb (L8)

I think introducing the `fields` concept is a good idea! However, with the current code structure, dynamic size calculation could be tricky. I'm not entirely sure how to handle it because the same partial should fit both a narrow widget and a wide page while keeping the columns aligned. Using a `<table>` could potentially fix this issue, but it might be a bit old-fashioned https://github.com/maybe-finance/maybe/blob/c46662c99f68fd1deab5680b0dd4deb31a20eda8/app/views/transactions/_transaction.html.erb#L3 https://github.com/maybe-finance/maybe/blob/c46662c99f68fd1deab5680b0dd4deb31a20eda8/app/views/transactions/_transaction.html.erb#L8
mattia-malnis commented 2024-04-20 16:12:27 +08:00 (Migrated from github.com)
Copy Link

@zachgoll I noticed there's an issue with the category dropdown (in the main branch):

  • When I update a category, the app crashes.
  • When I try to create a category, nothing happens if I leave the first color selected by default.
  • Checking the controller, I notice that every action performs a redirect to transactions_path, which could be misleading for a user who is on the dashboard or the account details page.

I think it's better to open a new issue for these problems.

immagine
@zachgoll I noticed there's an issue with the category dropdown (in the `main` branch): - When I update a category, the app crashes. - When I try to create a category, nothing happens if I leave the first color selected by default. - Checking the [controller](https://github.com/maybe-finance/maybe/blob/c46662c99f68fd1deab5680b0dd4deb31a20eda8/app/controllers/transactions/categories_controller.rb#L1), I notice that every action performs a redirect to `transactions_path`, which could be misleading for a user who is on the dashboard or the account details page. I think it's better to open a new issue for these problems. <img width="1470" alt="immagine" src="https://github.com/maybe-finance/maybe/assets/15941778/33fa1d63-13bf-4678-994e-5735778722f7">
zachgoll (Migrated from github.com) reviewed 2024-04-20 20:08:41 +08:00
app/views/transactions/_transaction.html.erb
@@ -1,17 +1,22 @@
<%# locals: (transaction:) %>
zachgoll (Migrated from github.com) commented 2024-04-20 20:08:41 +08:00
Copy Link

If it's too challenging I think it's okay if we keep it as-is. A table will introduce styling challenges of its own and make it less flexible to deal with if we start introducing inline turbo frame editing to this partial.

If it's too challenging I think it's okay if we keep it as-is. A table will introduce styling challenges of its own and make it less flexible to deal with if we start introducing inline turbo frame editing to this partial.
zachgoll commented 2024-04-20 20:45:46 +08:00 (Migrated from github.com)
Copy Link

@mattia-malnis yep, I'm seeing that bug with the categories too. I agree that we should open a separate issue for that and it's probably worth including all of them in one issue as an "Improvements" (even though technically there is a bug in there). Looks like we need to address the redirects behavior, some Turbo Streams, the bug, and a few others.

If you want to open up an issue outlining what you're seeing I can go back through and update it with any additional things that we might want to cover in it.

@mattia-malnis yep, I'm seeing that bug with the categories too. I agree that we should open a separate issue for that and it's probably worth including all of them in one issue as an "Improvements" (even though technically there is a bug in there). Looks like we need to address the redirects behavior, some Turbo Streams, the bug, and a few others. If you want to open up an issue outlining what you're seeing I can go back through and update it with any additional things that we might want to cover in it.
mattia-malnis commented 2024-04-21 00:49:25 +08:00 (Migrated from github.com)
Copy Link

@zachgoll This PR is ready for review!

I'm gonna open a separate issue regarding the categories dropdown issue, either later today or tomorrow

@zachgoll This PR is ready for review! I'm gonna open a separate issue regarding the categories dropdown issue, either later today or tomorrow
mattia-malnis commented 2024-04-21 21:54:14 +08:00 (Migrated from github.com)
Copy Link

I opened the issue related to the categories dropdown: #659

I opened the issue related to the categories dropdown: #659
👍 1
zachgoll (Migrated from github.com) reviewed 2024-04-22 20:27:15 +08:00
app/views/transactions/_transaction.html.erb
@@ -1,17 +1,22 @@
<%# locals: (transaction:) %>
zachgoll (Migrated from github.com) commented 2024-04-22 19:59:02 +08:00
Copy Link

In order for the Tailwind JIT compiler to scan and find all the static classes it needs to include, each class needs to be complete. The JIT compiler cannot execute code, so this could cause unexpected styles in prod:

https://tailwindcss.com/docs/content-configuration#dynamic-class-names

In order for the Tailwind JIT compiler to scan and find all the static classes it needs to include, each class needs to be complete. The JIT compiler cannot execute code, so this could cause unexpected styles in prod: https://tailwindcss.com/docs/content-configuration#dynamic-class-names
app/views/transactions/_transaction_group.html.erb
@@ -1,5 +1,8 @@
<%# locals: (date:, transactions:) %>
<div class="bg-gray-25 rounded-xl p-1">
<%# locals: (transaction_group:) %>
zachgoll (Migrated from github.com) commented 2024-04-22 20:23:46 +08:00
Copy Link

While I'm fine with the is_widget concept, I don't think we should be passing is_widget through so many partials (_transaction_group, _menu, _row) and storing in query params. Many of these partials don't need to know about this config and are simply passing it along.

I think we could isolate this piece of logic better by encapsulating it directly in the _transaction partial and using a helper to determine the setting. While this solution still doesn't feel like the best, it reduces the surface area for change and will allow us to update one file in the future if we do come up with a better solution (rather than having to pass locals to so many partials):

# transactions_helper.rb

def full_width_transaction_row?(route)
  route != "/"
end

<%# _transaction.html.erb %>

<%= debug full_width_transaction_row?(request.path) %> # If on dashboard, returns false, otherwise true
While I'm fine with the `is_widget` concept, I don't think we should be passing `is_widget` through so many partials (`_transaction_group`, `_menu`, `_row`) and storing in query params. Many of these partials don't need to know about this config and are simply passing it along. I think we could isolate this piece of logic better by encapsulating it directly in the `_transaction` partial and using a helper to determine the setting. While this solution still doesn't feel like the _best_, it reduces the surface area for change and will allow us to update one file in the future if we do come up with a better solution (rather than having to pass locals to so many partials): ```rb # transactions_helper.rb def full_width_transaction_row?(route) route != "/" end ``` ```erb <%# _transaction.html.erb %> <%= debug full_width_transaction_row?(request.path) %> # If on dashboard, returns false, otherwise true ```
mattia-malnis (Migrated from github.com) reviewed 2024-04-22 23:44:03 +08:00
app/views/transactions/_transaction_group.html.erb
@@ -1,5 +1,8 @@
<%# locals: (date:, transactions:) %>
<div class="bg-gray-25 rounded-xl p-1">
<%# locals: (transaction_group:) %>
mattia-malnis (Migrated from github.com) commented 2024-04-22 23:44:03 +08:00
Copy Link

I agree with you, I'm passing is_widget to many partials, and the code could be difficult to maintain. Unfortunately, I don't think that moving the logic inside a transactions_helper.rb will fix the problem:

1 - On the dashboard, request.path is "/";
2 - When I edit the transaction from the dashboard, request.path is "/transactions/:id";
3 - On the transactions page, request.path is "/transactions";
4 - When I edit the transaction from the transactions page, request.path is "/transactions/:id";

As you can see from points 2 and 4, request.path is the same (as we're pointing to transactions#update) but for point 2, we need the widget version, while for point 4, we need the full-width version.

Probably, this could be a solution (there could be some mistakes, I just want to give an idea):

# app/views/transactions/_transaction.html.erb

<%# locals: (transaction:, is_widget: "no") %>

<%= turbo_frame_tag dom_id(transaction), class:"text-gray-900 flex items-center gap-6 py-4 text-sm font-medium px-4" do %>
  <%= link_to transaction_path(transaction, is_widget:), data: { turbo_frame: "modal" }, class: "group", id: dom_id(transaction) do %>
    <%= render partial: "transactions/transaction_name" %>
  <% end %>
  <%= content_tag :div, class: "w-#{is_widget == 'yes' ? 36 : 48}" do %>
    <%= render partial: "transactions/categories/menu" %>
  <% end %>
  <%= render partial: "transactions/transaction_account_name" if is_widget == "no" %>
  <%= render partial: "transactions/transaction_amount" %>
<% end %>

# app/controllers/transactions_controller.rb

def update
  respond_to do |format|
    if @transaction.update(transaction_params)
      @transaction.account.sync_later

      format.html { redirect_to transaction_url(@transaction), notice: t(".success") }
      format.turbo_stream do
        render turbo_stream: [
          turbo_stream.append("notification-tray", partial: "shared/notification", locals: { type: "success", content: { body: t(".success") } }),
          turbo_stream.replace("transaction_name_#{@transaction.id}", partial: "transactions/transaction_name", locals: { transaction: @transaction }),
          turbo_stream.replace("transaction_category_#{@transaction.id}", partial: "transactions/categories/menu", locals: { transaction: @transaction }),
          turbo_stream.replace("transaction_account_#{@transaction.id}", partial: "transactions/transaction_account_name", locals: { transaction: @transaction }),
          turbo_stream.replace("transaction_amount_#{@transaction.id}", partial: "transactions/transaction_amount", locals: { transaction: @transaction }),
        ]
      end
    else
      format.html { render :edit, status: :unprocessable_entity }
    end
  end
end

By doing so, I can leave is_widget only on the _transaction partial. The controller will render all, and when we are on the dashboard, Turbo will skip the transactions/transaction_account_name partial because it won't find the corresponding id.
What do you think about that?

I agree with you, I'm passing `is_widget` to many partials, and the code could be difficult to maintain. Unfortunately, I don't think that moving the logic inside a `transactions_helper.rb` will fix the problem: 1 - On the dashboard, `request.path` is "/"; 2 - When I edit the transaction from the dashboard, `request.path` is "/transactions/:id"; 3 - On the transactions page, `request.path` is "/transactions"; 4 - When I edit the transaction from the transactions page, `request.path` is "/transactions/:id"; As you can see from points 2 and 4, `request.path` is the same (as we're pointing to `transactions#update`) but for point 2, we need the widget version, while for point 4, we need the full-width version. Probably, this could be a solution (there could be some mistakes, I just want to give an idea): ```ruby # app/views/transactions/_transaction.html.erb <%# locals: (transaction:, is_widget: "no") %> <%= turbo_frame_tag dom_id(transaction), class:"text-gray-900 flex items-center gap-6 py-4 text-sm font-medium px-4" do %> <%= link_to transaction_path(transaction, is_widget:), data: { turbo_frame: "modal" }, class: "group", id: dom_id(transaction) do %> <%= render partial: "transactions/transaction_name" %> <% end %> <%= content_tag :div, class: "w-#{is_widget == 'yes' ? 36 : 48}" do %> <%= render partial: "transactions/categories/menu" %> <% end %> <%= render partial: "transactions/transaction_account_name" if is_widget == "no" %> <%= render partial: "transactions/transaction_amount" %> <% end %> ``` ```ruby # app/controllers/transactions_controller.rb def update respond_to do |format| if @transaction.update(transaction_params) @transaction.account.sync_later format.html { redirect_to transaction_url(@transaction), notice: t(".success") } format.turbo_stream do render turbo_stream: [ turbo_stream.append("notification-tray", partial: "shared/notification", locals: { type: "success", content: { body: t(".success") } }), turbo_stream.replace("transaction_name_#{@transaction.id}", partial: "transactions/transaction_name", locals: { transaction: @transaction }), turbo_stream.replace("transaction_category_#{@transaction.id}", partial: "transactions/categories/menu", locals: { transaction: @transaction }), turbo_stream.replace("transaction_account_#{@transaction.id}", partial: "transactions/transaction_account_name", locals: { transaction: @transaction }), turbo_stream.replace("transaction_amount_#{@transaction.id}", partial: "transactions/transaction_amount", locals: { transaction: @transaction }), ] end else format.html { render :edit, status: :unprocessable_entity } end end end ``` By doing so, I can leave `is_widget` only on the `_transaction` partial. The controller will render all, and when we are on the dashboard, Turbo will skip the `transactions/transaction_account_name` partial because it won't find the corresponding id. What do you think about that?
zachgoll (Migrated from github.com) reviewed 2024-04-23 01:58:15 +08:00
app/views/transactions/_transaction_group.html.erb
@@ -1,5 +1,8 @@
<%# locals: (date:, transactions:) %>
<div class="bg-gray-25 rounded-xl p-1">
<%# locals: (transaction_group:) %>
zachgoll (Migrated from github.com) commented 2024-04-23 01:58:15 +08:00
Copy Link

@mattia-malnis got it, I understand what you're saying now. I was not considering the stream response that's being constructed after editing.

While long term I'd imagine we'll introduce editing from the dashboard, it sounds like if we made it "read only" on the dashboard we could use the helper and avoid this. Am I thinking about that correctly?

If so, I think we'd be totally fine to make this read only for now to avoid all the extra complexity.

@mattia-malnis got it, I understand what you're saying now. I was not considering the stream response that's being constructed after editing. While long term I'd imagine we'll introduce editing from the dashboard, it sounds like if we made it "read only" on the dashboard we could use the helper and avoid this. Am I thinking about that correctly? If so, I think we'd be totally fine to make this read only for now to avoid all the extra complexity.
mattia-malnis (Migrated from github.com) reviewed 2024-04-23 04:19:44 +08:00
app/views/transactions/_transaction_group.html.erb
@@ -1,5 +1,8 @@
<%# locals: (date:, transactions:) %>
<div class="bg-gray-25 rounded-xl p-1">
<%# locals: (transaction_group:) %>
mattia-malnis (Migrated from github.com) commented 2024-04-23 04:19:44 +08:00
Copy Link

@zachgoll I updated the code following your advice on using the full_width_transaction_row? helper and making the widget read-only!

@zachgoll I updated the code following your advice on using the `full_width_transaction_row?` helper and making the widget read-only!
zachgoll (Migrated from github.com) reviewed 2024-04-23 04:50:56 +08:00
app/views/transactions/_transaction_group.html.erb
@@ -1,5 +1,8 @@
<%# locals: (date:, transactions:) %>
<div class="bg-gray-25 rounded-xl p-1">
<%# locals: (transaction_group:) %>
zachgoll (Migrated from github.com) commented 2024-04-23 04:50:56 +08:00
Copy Link

Looks good!

Looks good!
Sign in to join this conversation.
Reviewers
No Reviewers
mattia-malnis
zachgoll
Labels
Clear labels
📆 Planned
Not ready to be worked on yet, but planned for future
 💻 Self Hosted only
Issues pertaining to self-hosted versions of Maybe
 ⚙️ Config
ℹ️ Needs info
Further information is requested
 📱 Mobile
Bugs exclusively happening on mobile devices
 1️⃣ High Priority
High priority issues will be worked on next
 🚨 Urgent
Issues that are urgent for app functionality
 3️⃣ Low priority
Contributions accepted, but Maybe team will not be working on this in the near term
 2️⃣ Medium Priority
Community contributions accepted, Maybe team only works on if there are no high priority items open
 ⚡ Performance
Performance issues to address
 🎨 UI / UX
Issues related to UI / UX interactions and design.
 💎 Bounty
💰 Rewarded
📊 Data issue
Issues where the app functionally works, but the data is missing or inaccurate.
 🙋 Bounty claim
$1.5K
Community
This is a great issue for community members to work on
 dependencies
Pull requests that update a dependency file
 Design
Hosted only
Hosted app only
 Required self hoster data migration
Self hosters must run data migrations to resolve this issue
 ruby
Pull requests that update Ruby code
No Label
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
gavin
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: gavin/maybe#656
Delete Branch "feature/transactions-widget"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?