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

Add additional subtypes, add None option, prefill edit with previously selected option. #1286

Merged
merged 13 commits into from
Oct 11, 2024

Conversation

ahatzz11
Copy link
Contributor

@ahatzz11 ahatzz11 commented Oct 10, 2024

  • "Investment" type
    • Add additional investment subtypes
    • Small reorder to put the 401k options next to each other
  • "Investment" and "Depository" type
    • Show the currently selected option as the default on the edit modal.
    • Add helper prompt text
    • Allow for "None" option

Edit window with initial state of previously selected option:
image

I've been in software for a decade but have never touched ruby, so using this as a slow learning opportunity

app/models/investment.rb Outdated Show resolved Hide resolved
@zachgoll
Copy link
Collaborator

@ahatzz11 thanks for the update. Agree w/the organization here.

For handling the nil value, I think we just need to remove that selected: "" option in the select input. Here's my diff:

diff --git a/app/views/accounts/accountables/_investment.html.erb b/app/views/accounts/accountables/_investment.html.erb
index d403d99..9d97ced 100644
--- a/app/views/accounts/accountables/_investment.html.erb
+++ b/app/views/accounts/accountables/_investment.html.erb
@@ -1 +1 @@
-<%= f.select :subtype, options_for_select(Investment::SUBTYPES, selected: ""), { label: true } %>
+<%= f.select :subtype, options_for_select(Investment::SUBTYPES), { label: true, prompt: t(".prompt") } %>
diff --git a/config/locales/views/accounts/en.yml b/config/locales/views/accounts/en.yml
index b0b6421..56b386a 100644
--- a/config/locales/views/accounts/en.yml
+++ b/config/locales/views/accounts/en.yml
@@ -5,6 +5,8 @@ en:
       has_issues: Issue detected.
       troubleshoot: Troubleshoot
     accountables:
+      investment:
+        prompt: Select subtype
       credit_card:
         annual_fee: Annual fee
         annual_fee_placeholder: '99'

@ahatzz11
Copy link
Contributor Author

ahatzz11 commented Oct 10, 2024

@ahatzz11 thanks for the update. Agree w/the organization here.

For handling the nil value, I think we just need to remove that selected: "" option in the select input. Here's my diff:

...

Thanks for the suggestion! I've gone ahead and made that change as well. I'll make a bug report for the state issue, but I appreciate the help so far!

@ahatzz11 ahatzz11 changed the title Add additional subtypes and allow for None Add additional subtypes, add None option, prefill edit with previously selected option. Oct 10, 2024
@zachgoll
Copy link
Collaborator

@ahatzz11 I see what you're saying now. I think I misread the first time around.

Rails forms should be able to populate this value automatically without much additional code. Let me take a deeper look at this and see what's going on.

@zachgoll
Copy link
Collaborator

@ahatzz11 okay I figured out the issue. To leverage some of the Rails form magic, we need to remove options_for_select and pass an array of options directly. Looks like we have to do this in two places:

<%# _depository.html.erb %>

<%= f.select :subtype, [["Checking", "checking"], ["Savings", "savings"]], { label: "Type", prompt: "Select subtype" } %>
<%# _investment.html.erb %>

<%= f.select :subtype, Investment::SUBTYPES, { label: true, prompt: "Select subtype" } %>

This should solve all the issues here:

  • Allows user to leave blank
  • Properly pre-selects the value already stored on the account

@ahatzz11
Copy link
Contributor Author

@zachgoll Hey Zach, after playing with this for awhile I don't think that quite works (but if I'm doing something wrong please let me know). Here is what I see when running your code for the subtypes:

  1. Create new account with Brokerage:
    image
  2. Edit account, no "None" option (but "Brokerage" is selected):
    image

I went back and forth on a bunch of options but for for the sake of not overcomplicating this I think the following should be true:

  • If no option is selected the helper prompt should be shown
  • If a subtype is chosen at creation or added later it should be shown when the edit screen pops up
  • There should be a "None" option for a user to select.

I did like how you removed the options_for_select and the associated selected though, that still seems to do the trick on using the current state. I took my original code and made that tweak from yours and this is now fulfilling all 3 of the bullets above:

<%= f.select :subtype, Investment::SUBTYPES, { label: true, prompt: t(".prompt"), include_blank: t(".none") } %>

I think the include_blank option is required here to ensure there is a blank option for the user to chose for nil. They could pick the "Select subtype" option, but I think that's kinda annoying.

I couldn't find any nice way to have non-selectable helpertext on this ruby form, so this is where I landed:
image
image

I also did this for depository! Happy to make any other changes you think would be good here.

Copy link
Collaborator

@zachgoll zachgoll left a comment

Choose a reason for hiding this comment

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

Yep, think these changes make sense! Thanks for the help on this one.

@zachgoll zachgoll merged commit c5bf1db into maybe-finance:main Oct 11, 2024
4 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