feat: [#5777] Add navigation buttons for steps in case contact form #5923

priyapower · 2024-07-17T02:58:45Z

What github issue is this PR for, if any?

Resolves #5777

👀 - This is my first contribution to this codebase, I am happy to make updates/change requests or discuss why I made certain decisions

❓ Open questions for a future PR:

"back" from the first step
- CURRENT BEHAVIOR: disabled
- Are you allowed to do this or should it be disabled?
- If yes, should it align with the expected validation behavior as the "Back" button at bottom of form?
"forward" from last step
- CURRENT BEHAVIOR: disabled
- Are you allowed to do this or should it be disabled?
- If you are allowed to "step forward", then would this just act as a secondary "submit" button?
Noticed a small style discrepancy that is out of scope for this work, but figured I should document - the dropdown is missing the padding on the right that other dropdowns have on this form
Noticed a possible bug with validation on the field "select all contact types"
- If you validate step 1 (details) successfully, but then go back and remove the field value, you can still validate successfully. I wonder if it is catching an empty array/list or something because it's multiselect within the dropdown (design question: why isn't this just a required checkbox set... why a dropdown at all?)

What changed, and why?

Added step buttons according to issue details (similar to carousel design and to the left of the phrase "step n of x"
🐛 Fixed a bug where after validation errors, the progress and step portion of the title disappeared because we didn't persist the data from #update
Handles validation/saving between pages when using step buttons (excluding details outlined in "Questions for a future PR" above)

How is this tested? (please write tests!) 💖💪

Note: if you see a flake in your test build in github actions, please post in slack #casa "Flaky test: " :) 💪
Note: We love capybara tests! If you are writing both haml/js and ruby, please try to test your work with tests at every level including system tests like https://github.com/rubyforgood/casa/tree/main/spec/system

Added a new spec for the new step button component spec/components/form/step_navigation_component_spec.rb

Screenshots please :)

Run your local server and take a screenshot of your work! Try to include the URL of the page as well as the contents of the page.

Feelings gif (optional)

What gif best describes your feeling working on this issue? https://giphy.com/
How to embed:

… until guidance from developers/product

priyapower · 2024-07-17T21:17:07Z

Gemfile.lock

@@ -570,9 +568,9 @@ GEM
 PLATFORMS
  arm64-darwin-21
  ruby
+  x86_64-darwin-19


Happy to trash this gemfile lock change, but figured I would flag that my recent dev setup triggered these changes

Yes you should drop this.

priyapower · 2024-07-17T21:17:32Z

app/components/form/step_navigation_component.html.erb

@@ -0,0 +1,34 @@
+<div>


new component for step navigation via buttons that resemble carousel designs (chevron buttons)

priyapower · 2024-07-17T21:17:57Z

app/components/form/step_navigation_component.html.erb

+        aria: { label: "Back step" },
+        disabled: !@nav_back do %>
+        <% if @nav_back.present? %>
+            <%= link_to @nav_back, title: "Back step", aria: { label: "Back step" } do %>


Chose to do a link within the button to have access to visible display of route/url on hover

priyapower · 2024-07-17T21:18:11Z

app/components/form/step_navigation_component.rb

@@ -0,0 +1,8 @@
+# frozen_string_literal: true
+
+class Form::StepNavigationComponent < ViewComponent::Base


New component

priyapower · 2024-07-17T21:18:32Z

app/components/form/title_component.html.erb

+        <% if @navigable %>
+          <%= render(@navigable) %>
+        <% end %>


Optional navigation buttons

priyapower · 2024-07-17T21:18:58Z

app/components/form/title_component.html.erb

+        <p class="col-auto">
+          <%= @steps_in_text %>
+        </p>
+        <div class="col">
+          <div class="progress">
+            <div class="progress-bar primary-bg" role="progressbar" style="width: <%= @progress %>%" aria-valuenow="<%= @progress %>" aria-valuemin="0" aria-valuemax="100"></div>


No change - just indent

priyapower · 2024-07-17T21:19:17Z

app/components/form/title_component.rb

@@ -1,11 +1,12 @@
 # frozen_string_literal: true

 class Form::TitleComponent < ViewComponent::Base
-  def initialize(title:, subtitle:, step: nil, total_steps: nil, notes: nil, autosave: false)
+  def initialize(title:, subtitle:, step: nil, total_steps: nil, notes: nil, autosave: false, navigable: nil)


new optional param for navigating through form via buttons near progress bar

This ends up with suuuuper long call to render method. How about we just use a slot? https://viewcomponent.org/guide/slots.html#component-slots

priyapower · 2024-07-17T21:19:49Z

app/controllers/case_contacts/form_controller.rb

@@ -9,29 +9,21 @@ class CaseContacts::FormController < ApplicationController

  # wizard_path
  def show
-    authorize @case_contact
+    manage_form_step


Use helper for repeated behavior

Leaving the authorize call in the action is better. Whether current_user is allowed to show/edit/update records is unrelated to the form step (mostly), and I want to see that a record has been authorized for this action, in the common pattern. Moving it to set_case_contact would make sense, but even that is too DRY for me, just leave it here.

priyapower · 2024-07-17T21:19:53Z

app/controllers/case_contacts/form_controller.rb

    render_wizard
    wizard_path
  end

  def update
-    authorize @case_contact
+    manage_form_step(step_navigation: true)


Use helper for repeated behavior

I'm a little confused what is happening here, seems like a lot of side effects are possible. also not clear how it interacts with some before_actions, which were already kind of a mess... does it make sense to check for steps in set_steps for example?

priyapower · 2024-07-17T21:20:17Z

app/controllers/case_contacts/form_controller.rb

-          else
-            render_wizard @case_contact, {}, {case_contact_id: @case_contact.id}
-          end
+          manage_navigation


New navigation behavior necessary for validating then routing to appropriate step

I don't like this because it doesn't look like a typical controller action, I'd like to see the response behavior in this method, instead of finding out what happens in manage_navigation...

What do you think of changing to smaller methods that are only responsible for setting variables, and leaving all the render/redirects logic in the action methods (show/update)?

priyapower · 2024-07-17T21:20:51Z

app/controllers/case_contacts/form_controller.rb

+    @page = wizard_steps.index(step) + 1
+    @total_pages = steps.count


This information is necessary for #update if you want to continue seeing the progress bar and navigation buttons after a form/validation error

This doesn't depend on any specific info, does it make more sense in a before action?

priyapower · 2024-07-17T21:20:58Z

app/controllers/case_contacts/form_controller.rb

+    authorize @case_contact
+    @page = wizard_steps.index(step) + 1
+    @total_pages = steps.count
+    @nav_step = params[:nav_step] if step_navigation


priyapower · 2024-07-17T21:21:50Z

app/controllers/case_contacts/form_controller.rb

+      jump_to(@nav_step.split("/").last.to_sym)
+      render_wizard @case_contact, {}, {case_contact_id: @case_contact.id}


Used wicked jump_to and then renders wizard to go to the specific step from navigation buttons

priyapower · 2024-07-17T21:22:31Z

app/views/case_contacts/form/details.html.erb

 <div>
  <%= form_with(model: @case_contact, url: wizard_path(nil, case_contact_id: @case_contact.id), local: true, id: "casa-contact-form", class: "component-validated-form") do |form| %>
+    <%= render(Form::TitleComponent.new(title: @case_contact.decorate.form_title, subtitle: "Contact details", step: @page, total_steps: @total_pages, navigable: Form::StepNavigationComponent.new(nav_back: nil, nav_next: next_wizard_path))) %>


Moved render under form so submit can catch

Added new parameter for navigable

priyapower · 2024-07-17T21:22:48Z

spec/components/form/step_navigation_component_spec.rb

+
+require "rails_helper"
+
+RSpec.describe Form::StepNavigationComponent, type: :component do


Testing new component for ability to enable or disable the buttons

elasticspoon

Haven't looked to close at the code yet but briefly testing it I seem to be able to proceed forward from the first page via arrow buttons and it ignores validations.

That is I can proceed via arrow key but not submit button.

elasticspoon · 2024-07-19T15:45:28Z

* "back" from the first step
  
  * CURRENT BEHAVIOR: disabled
  * Are you allowed to do this or should it be disabled?
  * If yes, should it align with the expected validation behavior as the "Back" button at bottom of form?

Disabled makes sense.

* "forward" from last step
  
  * CURRENT BEHAVIOR: disabled
  * Are you allowed to do this or should it be disabled?
  * If you are allowed to "step forward", then would this just act as a secondary "submit" button?

Disabled makes sense.

* Noticed a small style discrepancy that is out of scope for this work, but figured I should document - the dropdown is missing the padding on the right that other dropdowns have on this form
  ![Screen Shot 2024-07-17 at 3 51 41 PM](https://private-user-images.githubusercontent.com/49959312/349678525-3d2edd91-00d7-4924-ba53-64386ab10537.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjE0MDM2NzUsIm5iZiI6MTcyMTQwMzM3NSwicGF0aCI6Ii80OTk1OTMxMi8zNDk2Nzg1MjUtM2QyZWRkOTEtMDBkNy00OTI0LWJhNTMtNjQzODZhYjEwNTM3LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA3MTklMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwNzE5VDE1MzYxNVomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTMzMDIxZmZkMWI0OGI4ZGVhNWNmOTkxNDU4ODVjMGZhNzM4ZTc1NGRkZTA5NWRmYTVmN2ZmYWI1YjMyOTVmNjAmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.ggQFD40Ja71fkr-IIpmxh0suJzMbNW0z1Z-BqFd0hLs)

Good call. Feel free to open an issue 🙂

* Noticed a possible bug with validation on the field "select all contact types"
  
  * If you validate step 1 (details) successfully, but then go back and remove the field value, you can still validate successfully. I wonder if it is catching an empty array/list or something because it's multiselect within the dropdown (design question: why isn't this just a required checkbox set... why a dropdown at all?)

Good catch. I make a note and open an issue later. Or you can if you would like. And for context the previous design was multiple checkboxes but we got some feedback that there are too many (20+) in some orgs.

priyapower · 2024-07-19T15:46:00Z

Haven't looked to close at the code yet but briefly testing it I seem to be able to proceed forward from the first page via arrow buttons and it ignores validations.

That is I can proceed via arrow key but not submit button.

@elasticspoon

I will remove gemfile.lock change
I will make the slot update
I will start looking into the conditional bug for validation and submit... I tested that exact case so I will need a second to dig in (#whydoesitworkonmine 😆)

priyapower · 2024-07-22T20:07:05Z

@elasticspoon

✅ I will remove gemfile.lock change
✅ I will make the slot update
❌ I will start looking into the conditional bug for validation and submit... I tested that exact case so I will need a second to dig in (#whydoesitworkonmine 😆)
- I am unable to recreate "failed to validate on nav button" so I am still digging in here to figure it out why your local was able to click forward and avoid validation but mine catches validation. I'm going to try other test users and dig in further into the current forms code to see if I can spot why this behavior would have hit the exception for validation instead of the default of assuming validate on wizard movement
- Do you mind confirming again if this behavior is still live on this branch? Which dev/test user are you using?
❌ New spec failures are due to the slot change and updating the spec to reflect this new behavior - working on this update as well

thejonroberts

I am a bit confused. This controller was a mess before you got to it, so it makes all of this a little harder to reason through, not your fault! I also need to learn more about ViewComponents, but all of that side looks really good.

I think my major concerns are the indirection of manage_form_step and the lack of clarity of what @nav_step represents.

I may take a crack at refactoring this controller to clear it up and use more wizard helpers from the gem.

Let me know if you have any questions about my reviews!

thejonroberts · 2024-08-26T15:17:46Z

app/components/form/step_navigation_component.rb

+# frozen_string_literal: true
+
+class Form::StepNavigationComponent < ViewComponent::Base
+  def initialize(nav_back: nil, nav_next: nil)


Suggested change

def initialize(nav_back: nil, nav_next: nil)

def initialize(nav_back:, nav_next:)

I don't think nil defaults make sense for most cases? This would allow passing nil in explicitly if needed, but remind devs they need to specify the actions if they've forgotten!

thejonroberts · 2024-08-26T16:00:06Z

app/controllers/case_contacts/form_controller.rb

@@ -9,29 +9,21 @@ class CaseContacts::FormController < ApplicationController

  # wizard_path
  def show
-    authorize @case_contact
+    manage_form_step


Leaving the authorize call in the action is better. Whether current_user is allowed to show/edit/update records is unrelated to the form step (mostly), and I want to see that a record has been authorized for this action, in the common pattern. Moving it to set_case_contact would make sense, but even that is too DRY for me, just leave it here.

thejonroberts · 2024-08-26T16:31:06Z

app/components/form/step_navigation_component.html.erb

+        <% if @nav_back.present? %>
+            <%= link_to @nav_back, title: "Back step", aria: { label: "Back step" } do %>
+                <i class="lni lni-chevron-left" alt="Chevron icon left"></i>
+            <% end %>
+        <% elsif %>
+            <i class="lni lni-chevron-left" alt="Chevron icon left"></i>
+        <% end %>


disabled attribute above should disable the link, if so this would do:

Suggested change

<% if @nav_back.present? %>

<%= link_to @nav_back, title: "Back step", aria: { label: "Back step" } do %>



<% end %>

<% elsif %>



<% end %>

<%= link_to "#{@nav_back || '#'}", title: "Back step", aria: { label: "Back step" } do %>



<% end %>

thejonroberts · 2024-08-26T16:32:15Z

app/components/form/step_navigation_component.html.erb

+        value: @nav_back,
+        class: "btn btn-link #{@nav_back.nil? ? 'disabled' : 'enabled'}",
+        title: "Back step",
+        aria: { label: "Back step" },


Thank you for included aria attributes!

Suggested change

aria: { label: "Back step" },

aria: { label: "Back step", disabled: !@nav_back },

thejonroberts · 2024-08-26T17:25:59Z

app/components/form/step_navigation_component.html.erb

+    <%# Next navigation %>
+    <%= button_tag type: :submit,
+        name: :nav_step,
+        value: @nav_forward,


Suggested change

value: @nav_forward,

value: @nav_next,

right?

maybe back_path next_path are more descriptive names?

thejonroberts · 2024-08-26T18:00:29Z

app/controllers/case_contacts/form_controller.rb

    render_wizard
    wizard_path
  end

  def update
-    authorize @case_contact
+    manage_form_step(step_navigation: true)


I'm a little confused what is happening here, seems like a lot of side effects are possible. also not clear how it interacts with some before_actions, which were already kind of a mess... does it make sense to check for steps in set_steps for example?

thejonroberts · 2024-08-26T18:02:02Z

app/controllers/case_contacts/form_controller.rb

-          else
-            render_wizard @case_contact, {}, {case_contact_id: @case_contact.id}
-          end
+          manage_navigation


I don't like this because it doesn't look like a typical controller action, I'd like to see the response behavior in this method, instead of finding out what happens in manage_navigation...

What do you think of changing to smaller methods that are only responsible for setting variables, and leaving all the render/redirects logic in the action methods (show/update)?

thejonroberts · 2024-08-26T18:03:02Z

app/controllers/case_contacts/form_controller.rb

+    @page = wizard_steps.index(step) + 1
+    @total_pages = steps.count


This doesn't depend on any specific info, does it make more sense in a before action?

thejonroberts · 2024-08-26T18:04:16Z

app/controllers/case_contacts/form_controller.rb

@@ -65,6 +64,17 @@ def get_cases_and_contact_types
    @selected_contact_type_ids = @case_contact.contact_type_ids
  end

+  def manage_navigation
+    if @nav_step.present?
+      jump_to(@nav_step.split("/").last.to_sym)


I'm confused what @nav_step is... why not make it correspond to a wizard step? Is it pulling double duty as a step and as a path?

github-actions bot added dependencies Pull requests that update a dependency file ruby Pull requests that update Ruby code erb labels Jul 17, 2024

priyapower added 11 commits July 17, 2024 14:58

[rubyforgood#5777] Update lock with updates from bin/setup and bundle

a6e65f9

[rubyforgood#5777] Add prototype buttons with form step navigation

c81874d

[rubyforgood#5777] Update with appropriate icons

38c95b7

Move both buttons to left of "Step"

146fa54

[rubyforgood#5777] Add accessibility labels

404c4f2

[rubyforgood#5777] Ensure progress bar persists after validation

dfd5408

[rubyforgood#5777] Refactor step navigation into unique component

2e4fdce

[rubyforgood#5777] Solve bug around validation when navigating backwards

31f6059

[rubyforgood#5777] Remove debug point

e9ae652

[rubyforgood#5777] Fix missing instantiated object every now and then

5160d38

[rubyforgood#5777] Remove excess behavior

8b76177

priyapower force-pushed the priyapower/ISSUE-#5777/add-nav-buttons-to-wizard-form-contact branch from 370cd63 to 8b76177 Compare July 17, 2024 19:59

priyapower added 4 commits July 17, 2024 15:11

[rubyforgood#5777] 🧹

1bea453

[rubyforgood#5777] 🧹

77309c9

[rubyforgood#5777] Remove leave url for back navigation on first step…

2308e72

… until guidance from developers/product

[rubyforgood#5777] Add spec for step navigation component

11fba79

github-actions bot added the Tests! 🎉💖👏 label Jul 17, 2024

priyapower added 3 commits July 17, 2024 15:43

[rubyforgood#5777] Update existing spec after modifying component

032207d

[rubyforgood#5777] 🧹

f6e0073

[rubyforgood#5777] Refactor large method

5d249ac

priyapower marked this pull request as ready for review July 17, 2024 21:10

priyapower requested review from compwron, FireLemons, littleforest and elasticspoon as code owners July 17, 2024 21:10

priyapower changed the title ~~[#5777] Add navigation buttons for steps in case contact form~~ feat: [#5777] Add navigation buttons for steps in case contact form Jul 17, 2024

priyapower mentioned this pull request Jul 17, 2024

Add Back and Forth Buttons to Case Contact Form #5777

Open

priyapower commented Jul 17, 2024

View reviewed changes

elasticspoon requested changes Jul 19, 2024

View reviewed changes

priyapower added 8 commits July 22, 2024 11:23

[rubyforgood#5777] Reset gemfile lock

1f980b5

🧹

06f41e0

🧹

c665b64

Test

aef0fdf

Fix

c1ebc07

Revert

1f09e29

[rubyforgood#5777] Refactor with slot

744d82f

🧹

ee3feb3

thejonroberts reviewed Aug 26, 2024

View reviewed changes

		@@ -0,0 +1,8 @@
		# frozen_string_literal: true

		class Form::StepNavigationComponent < ViewComponent::Base

		@page = wizard_steps.index(step) + 1
		@total_pages = steps.count

		jump_to(@nav_step.split("/").last.to_sym)
		render_wizard @case_contact, {}, {case_contact_id: @case_contact.id}


		require "rails_helper"

		RSpec.describe Form::StepNavigationComponent, type: :component do

	def initialize(nav_back: nil, nav_next: nil)
	def initialize(nav_back:, nav_next:)

	aria: { label: "Back step" },
	aria: { label: "Back step", disabled: !@nav_back },

feat: [#5777] Add navigation buttons for steps in case contact form #5923

Are you sure you want to change the base?

feat: [#5777] Add navigation buttons for steps in case contact form #5923

Conversation

priyapower commented Jul 17, 2024 • edited Loading

What github issue is this PR for, if any?

What changed, and why?

How is this tested? (please write tests!) 💖💪

Screenshots please :)

Feelings gif (optional)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejonroberts Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticspoon left a comment

Choose a reason for hiding this comment

elasticspoon commented Jul 19, 2024

priyapower commented Jul 19, 2024

priyapower commented Jul 22, 2024

thejonroberts left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thejonroberts Aug 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

priyapower commented Jul 17, 2024 •

edited

Loading

thejonroberts Aug 26, 2024 •

edited

Loading

thejonroberts Aug 26, 2024 •

edited

Loading