Add nice formatting for subtypes on account list #2138

2025-04-21T23:48:40+08:00

ahatzz11 commented

2025-04-21 23:48:40 +08:00

(Migrated from github.com)

On the sidebar for accounts the subtype is currently shown with the humanize formatting. This doesn't work super well for some subtypes because it only capitalizes the first letter, and standard formatting for some subtypes are all caps (e.g. IRA).

This PR introduces a new helper that:

Uses titleize to have title formatting (for cases like "Mutual Fund" instead of "Mutual fund")
Adds some special cases (e.g. "HSA" instead of "Hsa" or "hsa", and "401(k)" instead of "401k"

Before and after:

On the sidebar for accounts the subtype is currently shown with the `humanize` formatting. This doesn't work super well for some subtypes because it only capitalizes the first letter, and standard formatting for some subtypes are all caps (e.g. IRA). This PR introduces a new helper that: - Uses `titleize` to have title formatting (for cases like "Mutual Fund" instead of "Mutual fund") - Adds some special cases (e.g. "HSA" instead of "Hsa" or "hsa", and "401(k)" instead of "401k" Before and after: <img width="785" alt="image" src="https://github.com/user-attachments/assets/2f389b7d-0ba8-44fa-b34c-3a34aad25429" />

❤️ 1

zachgoll (Migrated from github.com) reviewed 2025-04-22 02:02:04 +08:00

zachgoll (Migrated from github.com) left a comment

I agree with the formatting changes here, but I think we should try and keep these labels in one spot in the codebase. We're already defining them in each accountable class, such as for investment:

a7dfafc907/app/models/investment.rb (L4-L17)

Long term, I think there's likely a better solution than this, but to leverage these existing definitions + labels, we can define a little helper in the accountable concern and define a singleton method within our "Account Group" struct:

module Accountable
  # Define empty array to ensure all accountables have this defined
  SUBTYPES = [] 

  class_methods do
      # Given a subtype, look up the "label" for this accountable type
      def subtype_label_for(subtype)
        self::SUBTYPES.find { |subtype_label, subtype_value| subtype_value == subtype }&.first
      end
  end
end

Then over in balance_sheet.rb, we'd just pass it in:

AccountGroup.new(
  key: accountable.model_name.param_key,
  name: accountable.display_name,
  classification: accountable.classification,
  total: group_total,
  total_money: Money.new(group_total, currency),
  weight: classification_total.zero? ? 0 : group_total / classification_total.to_d * 100,
  missing_rates?: accounts.any? { |a| a.missing_rates? },
  color: accountable.color,
  accounts: accounts.map do |account|
    account.define_singleton_method(:weight) do
      classification_total.zero? ? 0 : account.converted_balance / classification_total.to_d * 100
    end

    # Define our subtype labeling logic
    account.define_singleton_method(:subtype_label) do
      accountable.subtype_label_for(account.subtype) || accountable.display_name
    end

    account
  end.sort_by(&:weight).reverse
)

Then the view simply becomes:

<%= tag.p account.subtype_label, class: "text-sm text-secondary truncate" %>

It's far from a perfect solution given we're mixing a little bit of model / view logic together, but I'd really like to keep the value / label pairs defined in a single spot for consistency and so when we make changes to the labels it only has to be changed once.

I agree with the formatting changes here, but I think we should try and keep these labels in one spot in the codebase. We're already defining them in each accountable class, such as for investment: https://github.com/maybe-finance/maybe/blob/a7dfafc9072b2986834a0b9fff60e8241b1d82f1/app/models/investment.rb#L4-L17 Long term, I think there's likely a better solution than this, but to leverage these existing definitions + labels, we can define a little helper in the accountable concern and define a singleton method within our "Account Group" struct: ```rb module Accountable # Define empty array to ensure all accountables have this defined SUBTYPES = [] class_methods do # Given a subtype, look up the "label" for this accountable type def subtype_label_for(subtype) self::SUBTYPES.find { |subtype_label, subtype_value| subtype_value == subtype }&.first end end end ``` Then over in `balance_sheet.rb`, we'd just pass it in: ```rb AccountGroup.new( key: accountable.model_name.param_key, name: accountable.display_name, classification: accountable.classification, total: group_total, total_money: Money.new(group_total, currency), weight: classification_total.zero? ? 0 : group_total / classification_total.to_d * 100, missing_rates?: accounts.any? { |a| a.missing_rates? }, color: accountable.color, accounts: accounts.map do |account| account.define_singleton_method(:weight) do classification_total.zero? ? 0 : account.converted_balance / classification_total.to_d * 100 end # Define our subtype labeling logic account.define_singleton_method(:subtype_label) do accountable.subtype_label_for(account.subtype) || accountable.display_name end account end.sort_by(&:weight).reverse ) ``` Then the view simply becomes: ```erb <%= tag.p account.subtype_label, class: "text-sm text-secondary truncate" %> ``` It's far from a perfect solution given we're mixing a little bit of model / view logic together, but I'd really like to keep the `value / label` pairs defined in a single spot for consistency and so when we make changes to the labels it only has to be changed once.

ahatzz11 commented

2025-04-22 02:33:26 +08:00

(Migrated from github.com)

@zachgoll I love this idea. I kind of started going down that path but wasn't really sure the best place to put that mapping. I made those changes! Appreciate the help 👍

The one note here is that some of them have changed a bit, like HSA is now Health Savings Account. I don't think this is a bad thing - it now matches 1:1 to the dropdown menu where people can select a subtype. Maybe this is a bit overly verbose, but at least for now it's an improvement. Some day if there's a 'short name' or something we want to add it would be possible.

@zachgoll I love this idea. I kind of started going down that path but wasn't really sure the best place to put that mapping. I made those changes! Appreciate the help 👍 The one note here is that some of them have changed a bit, like `HSA` is now `Health Savings Account`. I don't think this is a bad thing - it now matches 1:1 to the dropdown menu where people can select a subtype. Maybe this is a bit overly verbose, but at least for now it's an improvement. Some day if there's a 'short name' or something we want to add it would be possible.

zachgoll commented

2025-04-22 02:41:30 +08:00

(Migrated from github.com)

@ahatzz11 it would be a slight bit more work, but I think it would be reasonable to define a short/long version of each:

SUBTYPES = {
  hsa: { short: "HSA", long: "Health Savings Account" },
  401k: { short: "401 (k)", long: "401 (k)" }
}

There are likely a few views throughout the codebase we'd have to alter for select options, but I don't think too many:

<%# something like this %>
<%= form.select :subtype, Depository::SUBTYPES.map { |k, v| [v[:short], k] } ... %>

Totally up to you. I'm good with how it is now too!

@ahatzz11 it would be a slight bit more work, but I think it would be reasonable to define a short/long version of each: ```rb SUBTYPES = { hsa: { short: "HSA", long: "Health Savings Account" }, 401k: { short: "401 (k)", long: "401 (k)" } } ``` There are likely a few views throughout the codebase we'd have to alter for select options, but I don't think too many: ```erb <%# something like this %> <%= form.select :subtype, Depository::SUBTYPES.map { |k, v| [v[:short], k] } ... %> ``` Totally up to you. I'm good with how it is now too!

ahatzz11 commented

2025-04-22 03:23:46 +08:00

(Migrated from github.com)

@zachgoll Since I started this I might as well keep it going!

I now have a short/long string for each type that has a subtype (for consistency across all subtypes), and to support the new label on the /accounts page.

For the dropdown menus when creating the subtype and the /accounts page I use :long. For the balance sheet I use the :short since that sidebar is a bit shorter.

Balance sheet sidebar looks the same as I previously implemented (although an improvement here is now properties/depository subtypes also get the same treatment!):

Here are some screenshots validating there aren't any regressions in the dropdown selectors:

And here's a view of the /accounts page with the subtypes (I matched the styling from the balance sheet sidebar):

@zachgoll Since I started this I might as well keep it going! I now have a short/long string for each type that has a subtype (for consistency across all subtypes), and to support the new label on the `/accounts` page. For the dropdown menus when creating the subtype and the `/accounts` page I use `:long`. For the balance sheet I use the `:short` since that sidebar is a bit shorter. Balance sheet sidebar looks the same as I previously implemented (although an improvement here is now properties/depository subtypes also get the same treatment!): ![CleanShot-002497 2025-04-21 at 14 20 52@2x](https://github.com/user-attachments/assets/b3736483-feed-4e79-bee8-564d0b484f1f) Here are some screenshots validating there aren't any regressions in the dropdown selectors: ![CleanShot-002491 2025-04-21 at 14 18 23@2x](https://github.com/user-attachments/assets/0b2ebd52-8319-41c4-b3e8-ddb36b9384d9) And here's a view of the `/accounts` page with the subtypes (I matched the styling from the balance sheet sidebar): ![CleanShot-002495 2025-04-21 at 14 19 31@2x](https://github.com/user-attachments/assets/00ae8cf4-d129-4eb8-90c0-59eaaf8d28ff)

zachgoll (Migrated from github.com) reviewed 2025-04-22 03:49:56 +08:00

zachgoll (Migrated from github.com) left a comment

Nice! Labels look good. Just added a few potential simplifications.

app/models/account.rb

						
				@@ -158,0 +163,4 @@

				  # Get long version of the subtype label

				  def long_subtype_label

				    accountable_class.long_subtype_label_for(subtype) || accountable_class.display_name

				  end

zachgoll (Migrated from github.com) commented

2025-04-22 03:48:07 +08:00

Small nit, but accountable_class should automatically be available on each Account instance, so these could be simplified to:

def short_subtype_label
  accountable_class.short_subtype_label_for(subtype) || accountable_class.display_name
end

def long_subtype_label
  accountable_class.long_subtype_label_for(subtype) || accountable_class.display_name
end

Small nit, but `accountable_class` should automatically be available on each Account instance, so these could be simplified to: ```rb def short_subtype_label accountable_class.short_subtype_label_for(subtype) || accountable_class.display_name end def long_subtype_label accountable_class.long_subtype_label_for(subtype) || accountable_class.display_name end ```

app/models/balance_sheet.rb

						
				@@ -59,6 +59,7 @@ class BalanceSheet

				          account.define_singleton_method(:weight) do

zachgoll (Migrated from github.com) commented

2025-04-22 03:42:47 +08:00

If I'm not mistaken, now that you've got both of these defined on account.rb directly, we can remove these singleton overrides here and things will work fine in the sidebar.

If I'm not mistaken, now that you've got both of these defined on `account.rb` directly, we can remove these singleton overrides here and things will work fine in the sidebar.

app/views/accounts/_account.html.erb

						
				@@ -17,6 +17,9 @@

				          </p>

zachgoll (Migrated from github.com) commented

2025-04-22 03:41:03 +08:00

          <% if account.long_subtype_label %>

```suggestion <% if account.long_subtype_label %> ```

ahatzz11 commented

2025-04-23 01:31:34 +08:00

(Migrated from github.com)

@zachgoll thanks for the comments! I changed those things and cursor also decided to add a little test case. Let me know what you think!

zachgoll (Migrated from github.com) approved these changes 2025-04-23 01:34:03 +08:00

zachgoll (Migrated from github.com) left a comment

Looks great! Once tests pass I'll get it merged.

ahatzz11 commented

2025-04-23 01:44:22 +08:00

(Migrated from github.com)

Looks great! Once tests pass I'll get it merged.

Whoops - cursor wasn't very good about where that field on the model was, I did look for a bin/test but didn't realize that was just via bin/rails, good to know!

> Looks great! Once tests pass I'll get it merged. Whoops - cursor wasn't very good about where that field on the model was, I did look for a `bin/test` but didn't realize that was just via `bin/rails`, good to know!

👍 1

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: gavin/maybe#2138

				`@@ -59,6 +59,7 @@ class BalanceSheet`
				`account.define_singleton_method(:weight) do`