-
Notifications
You must be signed in to change notification settings - Fork 26
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
[Shared dev] [#162254] Open "Report an issue" in modal #4325
Conversation
@@ -28,6 +28,10 @@ | |||
top: 6px; | |||
} | |||
|
|||
#js--reportAnIssueModal { |
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.
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.
we have existing examples on the code, or you can look at bootstrap 2 docs
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.
left some comments
redirect_to_order_id = params[:redirect_to_order_id] | ||
|
||
if modal? | ||
render partial: "instruments/actions/issues", locals: { product: @product, order_detail: @order_detail, instrument_issue: @instrument_issue, redirect_to_order_id: } |
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.
try moving this back to the new
template
|
||
if @instrument_issue.send_notification | ||
redirect_to reservations_path, notice: text("create.success") | ||
if redirect_to_order_id.present? |
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.
let's move this into a method that returns the redirect path
in that method we can check modal? and ensure we have an order to redirect to
we can also add a code comment there if needed to clarify the expected code paths
@@ -28,6 +28,10 @@ | |||
top: 6px; | |||
} | |||
|
|||
#js--reportAnIssueModal { |
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.
we have existing examples on the code, or you can look at bootstrap 2 docs
629cc0a
to
7cdaa3b
Compare
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.
Nice work on the refactors!
if modal? | ||
render layout: false | ||
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.
I think line 11 should handle this
@@ -1,3 +1,5 @@ | |||
- modal_view = @redirect_to_order_id.present? |
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.
can we set this in the controller?
.row | ||
.span12 | ||
%h2= text("title") | ||
- url = facility_order_order_detail_issues_path(current_facility, @order_detail.order, @order_detail, redirect_to_order_id: @redirect_to_order_id) |
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.
can we set this in the controller?
= f.input :message, as: :text, input_html: { class: "input-xxlarge" } | ||
= f.submit text("submit"), class: "btn btn-primary" | ||
.modal-footer | ||
= modal_cancel_button text: "Cancel" |
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.
i18n please
describe "Report an Issue" do | ||
it "redirects to original order show" do | ||
find("h3", text: cross_core_reservation_order.facility.to_s, match: :first).click | ||
find("a", text: "Report an Issue").click |
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.
maybe put this into a within
block to make it explicit which order detail we're reporting an issue about
Release Notes
Screenshot
Accessibility