[WIP][SelectPanel]: Ensure formbuilder input/defaultvalue is available without opening the selectpanel #3340
Conversation
|
kendallgassner
temporarily deployed
to
preview
GitHub Actions
Inactive
kendallgassner
changed the title
| <span data-select-panel-inputs="true"> | ||
| <%= @form_builder.hidden_field(@input_name, multiple: multi_select?, skip_default_ids: true, value: @default_value) %> | ||
| </span> | ||
| <% end %> |
There was a problem hiding this comment.
Stop using form_builder input in ActionList. This is important since the ActionList input is only available once items are loaded.
Previously, we could only use form_builder with fetch_strategy: :local since the ActionList is loaded on page load using this strategy. This is problematic since most of our usecases are using remote or eventually_local fetch strategies.
There was a problem hiding this comment.
Stop using form_builder input in ActionList.
ActionMenu also accepts form_arguments and depends on ActionList rendering the hidden form inputs. Would we need to make similar changes in ActionMenu?
There are ~8 instances of ActionMenu with form_arguments and ~3 instances of SelectPanel with form_arguments, so I think we'll need to tread carefully and make sure those existing instances don't break.
There was a problem hiding this comment.
-
If we're expanding support for
form_argumentsto non-local strategies, would we also want to account for multi-select and support multiple initial hidden input name/values? (multi-select form example) -
Just an observation.. in the EMU select panel instance, the
input_namethat's passed into the component is overridden with a different name when the action list opens. I was confused by that. The name,input_nameis kind of ambiguous and it's not clear to me if it's supposed to be used as the initial hidden input name similar to thedefault_valueyou're suggesting here.
| # happens in the view that renders the `SelectPanel`, which means the form builder object but isn't available in the | ||
| # view that renders the list items. In such a case, it can be useful to create an instance of the form builder maually: | ||
| # | ||
| # ```erb |
There was a problem hiding this comment.
Remove inaccurate logic.
| @@ -375,7 +355,7 @@ def with_avatar_item(**system_arguments) | |||
| # @param dynamic_aria_label_prefix [String] If provided, the prefix is prepended to the dynamic label and set as the value of the `aria-label` attribute on the show button. | |||
| # @param body_id [String] The unique ID of the panel body. If not provided, the body ID will be set to the panel ID with a "-body" suffix. | |||
| # @param list_arguments [Hash] Arguments to pass to the underlying <%= link_to_component(Primer::Alpha::ActionList) %> component. Only has an effect for the local fetch strategy. | |||
| # @param form_arguments [Hash] Form arguments to pass to the underlying <%= link_to_component(Primer::Alpha::ActionList) %> component. Only has an effect for the local fetch strategy. | |||
There was a problem hiding this comment.
Not anymore ✨ !
|
|
||
| @form_builder = form_arguments[:builder] | ||
| @default_value = form_arguments[:default_value] | ||
| @input_name = form_arguments[:name] |
There was a problem hiding this comment.
Extract all form_arguments and add an optional argument default_value so that remote and eventually_local strategies can pass in a pre-set value.
| @@ -984,7 +985,7 @@ export class SelectPanelElement extends HTMLElement { | |||
|
|
|||
| for (const selectedItem of this.selectedItems) { | |||
| const newInput = document.createElement('input') | |||
| newInput.setAttribute('data-list-input', 'true') | |||
| newInput.setAttribute('data-select-panel-input', 'true') | |||
There was a problem hiding this comment.
Change the search term so we don't accidentally look for ActionList's form_builder input.
| # @snapshot interactive | ||
| # @param open_on_load toggle | ||
| # @param selected_items text | ||
| def remote_fetch_form(open_on_load: false, selected_items: "Phaser") |
There was a problem hiding this comment.
TODO:
add an exentually_local fetch strategy
Authors: Please fill out this form carefully and completely.
Reviewers: By approving this Pull Request you are approving the code change, as well as its deployment and mitigation plans.
Please read this description carefully. If you feel there is anything unclear or missing, please ask for updates.
What are you trying to accomplish?
Screenshots
Integration
List the issues that this change affects.
Closes # (type the GitHub issue number after #)
Risk Assessment
What approach did you choose and why?
Anything you want to highlight for special attention from reviewers?
Accessibility
Merge checklist
Take a look at the What we look for in reviews section of the contributing guidelines for more information on how we review PRs.