4780: Fix item lookup issues on new kit page #4781
Conversation
|
Looks good to me from a funcitonal pov. Over to @dorner for technical comments |
app/helpers/ui_helper.rb
Outdated
| @@ -14,6 +14,7 @@ def add_element_button(label, container_selector:, **html_attrs, &block) | |||
| add_dest_selector: container_selector, | |||
| action: "click->form-input#addItem:prevent" }, | |||
| role: "button", | |||
| id: "__add_line_item", | |||
There was a problem hiding this comment.
This assumes we'd never have more than one "add element button" on a page. Can we key off something other than ID?
There was a problem hiding this comment.
Fair point. I will move the id back to being passed in as an argument.
While we could change it to being an .add_line_item class here, we would have to modify the JS that clicks this button automatically to find the element by class rather than id. And then use some logic to determine which (or all) of the add element buttons to click. Which feels like coding for some hypothetical future case of having mutliple add item buttons, which doesn't exist yet.
There was a problem hiding this comment.
Moved this id back to being an argument.
| @@ -22,7 +22,7 @@ | |||
| </div> | |||
| <div class="row links"> | |||
| <div class="col-xs-12 p-3"> | |||
| <%= add_element_button "Add Another Item", container_selector: "#audit_line_items" do %> | |||
| <%= add_element_button "Add Another Item", container_selector: "#audit_line_items", id: "__add_line_item" do %> | |||
There was a problem hiding this comment.
Noticed that this method call was also missing the id option.
I added some corresponding system specs, but I can remove them if you think the coverage isn't worth the extra weight of system specs.
|
@coalest: Your PR |
Resolves #4780
Description
The new kit view was missing the bootstrap modal to add new barcode items, and the proper HTML id in order for a new item to be adding automatically after adding an item via barcode.
Note: Because the JS relies on the add another item button to have a certain id, I moved that into the helper method, for future error proofing.
Type of change
How Has This Been Tested?
Added a new system test. (Not sure how else to test this behavior because it relies on JS)