-
-
Notifications
You must be signed in to change notification settings - Fork 486
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
feat: [#5777] Add navigation buttons for steps in case contact form #5923
Changes from all commits
a6e65f9
c81874d
38c95b7
146fa54
404c4f2
dfd5408
2e4fdce
31f6059
e9ae652
5160d38
8b76177
1bea453
77309c9
2308e72
11fba79
032207d
f6e0073
5d249ac
1f980b5
06f41e0
c665b64
aef0fdf
c1ebc07
1f09e29
744d82f
ee3feb3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,34 @@ | ||||||||||||||||||||||
<div> | ||||||||||||||||||||||
<%# Back navigation %> | ||||||||||||||||||||||
<%= button_tag type: :submit, | ||||||||||||||||||||||
name: :nav_step, | ||||||||||||||||||||||
value: @nav_back, | ||||||||||||||||||||||
class: "btn btn-link #{@nav_back.nil? ? 'disabled' : 'enabled'}", | ||||||||||||||||||||||
title: "Back step", | ||||||||||||||||||||||
aria: { label: "Back step" }, | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thank you for included aria attributes!
Suggested change
|
||||||||||||||||||||||
disabled: !@nav_back do %> | ||||||||||||||||||||||
<% if @nav_back.present? %> | ||||||||||||||||||||||
<%= link_to @nav_back, title: "Back step", aria: { label: "Back step" } do %> | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Chose to do a link within the button to have access to visible display of route/url on hover |
||||||||||||||||||||||
<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 %> | ||||||||||||||||||||||
Comment on lines
+10
to
+16
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. disabled attribute above should disable the link, if so this would do:
Suggested change
|
||||||||||||||||||||||
<% end %> | ||||||||||||||||||||||
<%# Next navigation %> | ||||||||||||||||||||||
<%= button_tag type: :submit, | ||||||||||||||||||||||
name: :nav_step, | ||||||||||||||||||||||
value: @nav_forward, | ||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
right? maybe |
||||||||||||||||||||||
class: "btn btn-link #{@nav_next.nil? ? 'disabled' : 'enabled'}", | ||||||||||||||||||||||
title: "Next step", | ||||||||||||||||||||||
aria: { label: "Next step" }, | ||||||||||||||||||||||
disabled: !@nav_next do %> | ||||||||||||||||||||||
<% if @nav_next.present? %> | ||||||||||||||||||||||
<%= link_to @nav_next, title: "Next step", aria: { label: "Next step" } do %> | ||||||||||||||||||||||
<i class="lni lni-chevron-right" alt="Chevron icon right"></i> | ||||||||||||||||||||||
<% end %> | ||||||||||||||||||||||
<% elsif %> | ||||||||||||||||||||||
<i class="lni lni-chevron-right" alt="Chevron icon right"></i> | ||||||||||||||||||||||
<% end %> | ||||||||||||||||||||||
<% end %> | ||||||||||||||||||||||
</div> |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,8 @@ | ||||||
# frozen_string_literal: true | ||||||
|
||||||
class Form::StepNavigationComponent < ViewComponent::Base | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New component |
||||||
def initialize(nav_back: nil, nav_next: nil) | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
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! |
||||||
@nav_back = nav_back | ||||||
@nav_next = nav_next | ||||||
end | ||||||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,15 +11,18 @@ | |
<h2 class="col-12 col-md-6 case-contacts-form-subtitle"><%= @subtitle %></h2> | ||
<% if @progress %> | ||
<div class="col-12 col-md-6 align-items-center d-flex gap-2 mt-2 mt-md-0"> | ||
<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> | ||
</div> | ||
<% if navigable %> | ||
<%= navigable %> | ||
<% end %> | ||
<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> | ||
Comment on lines
+17
to
+22
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No change - just indent |
||
</div> | ||
</div> | ||
</div> | ||
<% end %> | ||
</div> | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,29 +9,21 @@ class CaseContacts::FormController < ApplicationController | |
|
||
# wizard_path | ||
def show | ||
authorize @case_contact | ||
manage_form_step | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use helper for repeated behavior There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Leaving the |
||
get_cases_and_contact_types | ||
@page = wizard_steps.index(step) + 1 | ||
@total_pages = steps.count | ||
|
||
render_wizard | ||
wizard_path | ||
end | ||
|
||
def update | ||
authorize @case_contact | ||
manage_form_step(step_navigation: true) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Use helper for repeated behavior There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
params[:case_contact][:status] = step.to_s if !@case_contact.active? | ||
remove_unwanted_contact_types | ||
remove_nil_draft_ids | ||
|
||
if CaseContactUpdateService.new(@case_contact).update_attrs(case_contact_params) | ||
respond_to do |format| | ||
format.html { | ||
if step == steps.last | ||
finish_editing | ||
else | ||
render_wizard @case_contact, {}, {case_contact_id: @case_contact.id} | ||
end | ||
manage_navigation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New navigation behavior necessary for validating then routing to appropriate step There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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)? |
||
} | ||
format.json { head :ok } | ||
end | ||
|
@@ -52,6 +44,13 @@ def set_case_contact | |
@case_contact = CaseContact.find(params[:case_contact_id]) | ||
end | ||
|
||
def manage_form_step(step_navigation: false) | ||
authorize @case_contact | ||
@page = wizard_steps.index(step) + 1 | ||
@total_pages = steps.count | ||
Comment on lines
+49
to
+50
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This information is necessary for #update if you want to continue seeing the progress bar and navigation buttons after a form/validation error There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't depend on any specific info, does it make more sense in a before action? |
||
@nav_step = params[:nav_step] if step_navigation | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. New value |
||
end | ||
|
||
def get_cases_and_contact_types | ||
@casa_cases = policy_scope(current_organization.casa_cases) | ||
@casa_cases = @casa_cases.where(id: @case_contact.casa_case_id) if @case_contact.active? | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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? |
||
render_wizard @case_contact, {}, {case_contact_id: @case_contact.id} | ||
Comment on lines
+69
to
+70
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Used wicked |
||
elsif step == steps.last | ||
finish_editing | ||
else | ||
render_wizard @case_contact, {}, {case_contact_id: @case_contact.id} | ||
end | ||
end | ||
|
||
def finish_editing | ||
message = "" | ||
send_reimbursement_email(@case_contact) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
# frozen_string_literal: true | ||
|
||
require "rails_helper" | ||
|
||
RSpec.describe Form::StepNavigationComponent, type: :component do | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Testing new component for ability to enable or disable the buttons |
||
context "can handle enabled button" do | ||
it "enables button if value exists" do | ||
render_inline(described_class.new(nav_back: "/details")) | ||
expect(page).to have_selector(:link_or_button, "Back step") | ||
end | ||
end | ||
|
||
context "can handle disabled button" do | ||
it "disables buttons if value is nil" do | ||
render_inline(described_class.new) | ||
expect(page).not_to have_selector(:link_or_button, "Next step") | ||
end | ||
end | ||
end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
new component for step navigation via buttons that resemble carousel designs (chevron buttons)