From 725bbd954564995667398b1ea0b1388d4f6d8410 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Wed, 18 Oct 2023 15:29:18 -0700 Subject: [PATCH] Allow ActionMenu items to submit multiple values on form submission; fix keyboard handling for submit items (#2291) --- .changeset/pink-wolves-fix.md | 7 + Gemfile | 3 +- Gemfile.lock | 5 +- .../alpha/action_list/form_wrapper.html.erb | 6 +- .../primer/alpha/action_list/form_wrapper.rb | 29 +++-- app/components/primer/alpha/action_menu.rb | 123 +++++++++++++++++- .../alpha/action_menu/action_menu_element.ts | 6 - .../app/controllers/action_menu_controller.rb | 15 ++- .../views/action_menu/form_action.html.erb | 3 +- lib/primer/static/generate_info_arch.rb | 91 ++++++++++++- previews/primer/alpha/action_menu_preview.rb | 10 +- .../single_select_form_items.html.erb | 31 +++++ .../action_menu_preview/with_actions.html.erb | 2 +- test/system/alpha/action_menu_test.rb | 53 +++++++- 14 files changed, 350 insertions(+), 34 deletions(-) create mode 100644 .changeset/pink-wolves-fix.md create mode 100644 previews/primer/alpha/action_menu_preview/single_select_form_items.html.erb diff --git a/.changeset/pink-wolves-fix.md b/.changeset/pink-wolves-fix.md new file mode 100644 index 0000000000..4547070c97 --- /dev/null +++ b/.changeset/pink-wolves-fix.md @@ -0,0 +1,7 @@ +--- +'@primer/view-components': minor +--- + +Allow ActionMenu items to submit multiple values on form submission; fix keyboard handling for submit items + + diff --git a/Gemfile b/Gemfile index 2c6a5f53a8..1b93dc3ecc 100644 --- a/Gemfile +++ b/Gemfile @@ -41,7 +41,8 @@ gem "bootsnap", ">= 1.4.2", require: false gem "lookbook", "~> 2.1.1" unless rails_version.to_f < 7 gem "view_component", path: ENV["VIEW_COMPONENT_PATH"] if ENV["VIEW_COMPONENT_PATH"] -gem "sourcemap" +gem "sourcemap", "~> 0.1" +gem "kramdown", "~> 2.4" group :test do gem "webmock" diff --git a/Gemfile.lock b/Gemfile.lock index 1a16b73392..a9f2d124e6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -87,6 +87,8 @@ GEM htmlentities (4.3.4) i18n (1.12.0) concurrent-ruby (~> 1.0) + kramdown (2.4.0) + rexml listen (3.8.0) rb-fsevent (~> 0.10, >= 0.10.3) rb-inotify (~> 0.9, >= 0.9.10) @@ -249,6 +251,7 @@ DEPENDENCIES cuprite (~> 0.14.3) erb_lint (~> 0.4.0) erblint-github (= 0.4.0) + kramdown (~> 2.4) listen (~> 3.0) lookbook (~> 2.1.1) matrix (~> 0.4.2) @@ -266,7 +269,7 @@ DEPENDENCIES rubocop-performance (~> 1.7) simplecov (~> 0.21.2) simplecov-console (~> 0.9.1) - sourcemap + sourcemap (~> 0.1) sprockets sprockets-rails thor diff --git a/app/components/primer/alpha/action_list/form_wrapper.html.erb b/app/components/primer/alpha/action_list/form_wrapper.html.erb index 71443377ae..eef78ef634 100644 --- a/app/components/primer/alpha/action_list/form_wrapper.html.erb +++ b/app/components/primer/alpha/action_list/form_wrapper.html.erb @@ -1,7 +1,9 @@ <% if form_required? %> <%= form_with(url: @action, method: @http_method, **@form_arguments) do %> - <% if render_input? %> - <%= render(Primer::BaseComponent.new(tag: :input, **@input_arguments)) %> + <% if render_inputs? %> + <% @inputs.each do |input_arguments| %> + <%= render(Primer::BaseComponent.new(tag: :input, **input_arguments)) %> + <% end %> <% end %> <%= content %> <% end %> diff --git a/app/components/primer/alpha/action_list/form_wrapper.rb b/app/components/primer/alpha/action_list/form_wrapper.rb index 7ce1fc68d6..b44e98f5c8 100644 --- a/app/components/primer/alpha/action_list/form_wrapper.rb +++ b/app/components/primer/alpha/action_list/form_wrapper.rb @@ -24,14 +24,25 @@ def initialize(list:, action: nil, **form_arguments) name = @form_arguments.delete(:name) value = @form_arguments.delete(:value) || name + inputs = @form_arguments.delete(:inputs) || [] - @input_arguments = { - type: :hidden, - name: name, - value: value, - data: { list_item_input: true }, - **(@form_arguments.delete(:input_arguments) || {}) - } + # For the older version of this component that only allowed you to + # specify a single input + if inputs.empty? + inputs << { + name: name, + value: value, + **(@form_arguments.delete(:input_arguments) || {}) + } + end + + @inputs = inputs.map do |input_data| + input_data = input_data.dup + input_data[:type] ||= :hidden + input_data[:data] ||= {} + input_data[:data][:list_item_input] = true + input_data + end end def get? @@ -42,8 +53,8 @@ def form_required? @action && !get? end - def render_input? - @input_arguments[:name].present? + def render_inputs? + @inputs.present? end private diff --git a/app/components/primer/alpha/action_menu.rb b/app/components/primer/alpha/action_menu.rb index be7c767708..a87eb0f1ea 100644 --- a/app/components/primer/alpha/action_menu.rb +++ b/app/components/primer/alpha/action_menu.rb @@ -3,14 +3,131 @@ module Primer module Alpha - # ActionMenu is used for actions, navigation, to display secondary options, or single/multi select lists. They appear when users interact with buttons, actions, or other controls. + # ActionMenu is used for actions, navigation, to display secondary options, or single/multi select lists. They appear when + # users interact with buttons, actions, or other controls. # # The only allowed elements for the `Item` components are: `:a`, `:button`, and `:clipboard-copy`. The default is `:button`. # + # ### Select variants + # + # While `ActionMenu`s default to a list of buttons that can link to other pages, copy text to the clipboard, etc, they also support + # `single` and `multiple` select variants. The single select variant allows a single item to be "selected" (i.e. marked "active") + # when clicked, which will cause a check mark to appear to the left of the item text. When the `multiple` select variant is chosen, + # multiple items may be selected and check marks will appear next to each selected item. + # + # Use the `select_variant:` option to control which variant the `ActionMenu` uses. For more information, see the documentation on + # supported arguments below. + # + # ### Dynamic labels + # + # When using the `single` select variant, an optional label indicating the selected item can be displayed inside the menu button. + # Dynamic labels can also be prefixed with custom text. + # + # Pass `dynamic_label: true` to enable dynamic label behavior, and pass `dynamic_label_prefix: ""` to set a custom prefix. + # For more information, see the documentation on supported arguments below. + # + # ### `ActionMenu`s as form inputs + # + # When using either the `single` or `multiple` select variants, `ActionMenu`s can be used as form inputs. They behave very + # similarly to how HTML `` element.| + # |`inputs` |`Array` |`[]` |An array of hashes representing HTML `` elements. Must contain at least `name:` and `value:` keys. If additional key/value pairs are provided, they are emitted as HTML attributes on the `` element. This argument supercedes the `name:`, `value:`, and `:input_arguments` arguments listed above.| + # + # The elements of the `inputs:` array will be emitted as HTML `` elements. + # # @accessibility - # The action for the menu item needs to be on the element with `role="menuitem"`. Semantics are removed for everything nested inside of it. When a menu item is selected, the menu will close immediately. + # The action for the menu item needs to be on the element with `role="menuitem"`. Semantics are removed for everything + # nested inside of it. When a menu item is selected, the menu will close immediately. # - # Additional information around the keyboard functionality and implementation can be found on the [WAI-ARIA Authoring Practices](https://www.w3.org/TR/wai-aria-practices-1.2/#menu). + # Additional information around the keyboard functionality and implementation can be found on the + # [WAI-ARIA Authoring Practices](https://www.w3.org/TR/wai-aria-practices-1.2/#menu). class ActionMenu < Primer::Component status :alpha diff --git a/app/components/primer/alpha/action_menu/action_menu_element.ts b/app/components/primer/alpha/action_menu/action_menu_element.ts index bb696725f2..f13539c9ce 100644 --- a/app/components/primer/alpha/action_menu/action_menu_element.ts +++ b/app/components/primer/alpha/action_menu/action_menu_element.ts @@ -283,12 +283,6 @@ export class ActionMenuElement extends HTMLElement { } this.#updateInput() - - if (event instanceof KeyboardEvent && event.target instanceof HTMLButtonElement) { - // prevent buttons from being clicked twice - event.preventDefault() - return - } } #activateItem(event: Event, item: Element) { diff --git a/demo/app/controllers/action_menu_controller.rb b/demo/app/controllers/action_menu_controller.rb index b2c208c602..34ddd9a62f 100644 --- a/demo/app/controllers/action_menu_controller.rb +++ b/demo/app/controllers/action_menu_controller.rb @@ -18,10 +18,11 @@ def form_action respond_to do |format| format.html do @value = form_action_selected_value + @other_params = form_action_other_params end format.json do - render json: { value: form_action_selected_value } + render json: { value: form_action_selected_value, other_params: form_action_other_params } end end end @@ -31,4 +32,16 @@ def form_action def form_action_selected_value params.permit(:foo)[:foo] || params.permit(foo: [])[:foo] end + + def form_action_other_params + params.permit!.to_hash.tap do |all| + case all + when Hash + all.delete("foo") + all.delete("authenticity_token") + when Array + all.delete(form_action_selected_value) + end + end + end end diff --git a/demo/app/views/action_menu/form_action.html.erb b/demo/app/views/action_menu/form_action.html.erb index f80e7fc2c2..4004815c8b 100644 --- a/demo/app/views/action_menu/form_action.html.erb +++ b/demo/app/views/action_menu/form_action.html.erb @@ -1 +1,2 @@ -You selected <%= @value.inspect %> +You selected <%= @value.inspect %>
+Other params sent to server: <%= @other_params.inspect %> diff --git a/lib/primer/static/generate_info_arch.rb b/lib/primer/static/generate_info_arch.rb index d3c5356408..9d5e71e59e 100644 --- a/lib/primer/static/generate_info_arch.rb +++ b/lib/primer/static/generate_info_arch.rb @@ -3,6 +3,7 @@ # :nocov: require "json" +require "kramdown" module Primer module Static @@ -35,7 +36,7 @@ def call # rubocop:disable Style/IfUnlessModifier "description" => if slot_method.base_docstring.to_s.present? - view_context.render(inline: slot_method.base_docstring) + render_erb_ignoring_markdown_code_fences(slot_method.base_docstring) end, # rubocop:enable Style/IfUnlessModifier "parameters" => serialize_params(param_tags, component) @@ -57,7 +58,7 @@ def call { "name" => mtd.name, - "description" => view_context.render(inline: mtd.base_docstring), + "description" => render_erb_ignoring_markdown_code_fences(mtd.base_docstring), "parameters" => serialize_params(param_tags, component) } end @@ -66,7 +67,7 @@ def call if component == Primer::BaseComponent docs.base_docstring else - view_context.render(inline: docs.base_docstring) + render_erb_ignoring_markdown_code_fences(docs.base_docstring) end memo[component] = { @@ -119,7 +120,7 @@ def system_args_docs component: "BaseComponent", fully_qualified_name: "Primer::BaseComponent", description_md: docs.base_docstring, - args_md: view_context.render(inline: docs.constructor.base_docstring) + args_md: render_erb_ignoring_markdown_code_fences(docs.constructor.base_docstring) } end @@ -131,7 +132,7 @@ def serialize_params(param_tags, component) "name" => tag.name, "type" => tag.types&.join(", ") || "", "default" => default_value, - "description" => view_context.render(inline: tag.text.squish) + "description" => render_erb_ignoring_markdown_code_fences(tag.text.squish) } end end @@ -151,6 +152,86 @@ def view_context end end + # Renders ERB code to a string, ignoring markdown code fences. For example, consider the + # following ERB code inside a markdown document: + # + # ### Heading + # ```erb + # <%= render(SomeComponent.new) %> + # ``` + # + # <%= some_func(a, b) %> + # + # The ERB renderer does not understand that the fenced code, i.e. the part inside the triple + # backticks, should not be rendered. It sees the ERB tags both inside and outside the fence + # and renders them both. + # + # This method renders ERB tags in a markdown string, ignoring any fenced code blocks, so as + # to prevent rendering fenced ERB code. + # + def render_erb_ignoring_markdown_code_fences(markdown_str) + return view_context.render(inline: markdown_str) unless markdown_str.include?("```") + + # identify all fenced code blocks in markdown string + code_ranges = find_fenced_code_ranges_in(markdown_str) + + # replace code fences with placeholders + de_fenced_markdown_str = markdown_str.dup.tap do |memo| + code_ranges.reverse_each.with_index do |code_range, idx| + memo[code_range] = "" + end + end + + # Render ERB tags. The only ones left will explicitly exist _outside_ markdown code fences. + rendered_str = view_context.render(inline: de_fenced_markdown_str) + + # replace placeholders with original code fences + code_ranges.reverse_each.with_index do |code_range, idx| + rendered_str.sub!("", markdown_str[code_range]) + end + + rendered_str + end + + def find_fenced_code_ranges_in(str) + doc = Kramdown::Document.new(str) + line_starts = find_line_starts_in(str) + + [].tap do |code_ranges| + each_codespan_in(doc.root) do |node| + options = node.options + delimiter = options[:codespan_delimiter] + next unless delimiter.start_with?("```") + + start_pos = line_starts[options[:location]] + end_pos = start_pos + node.value.size + delimiter.size + end_pos = str.index("```", end_pos) + 3 + + code_ranges << (start_pos...end_pos) + end + end + end + + def find_line_starts_in(str) + line_counter = 2 + + { 1 => 0 }.tap do |memo| + str.scan(/\r?\n/) do + memo[line_counter] = Regexp.last_match.end(0) + line_counter += 1 + end + end + end + + def each_codespan_in(node, &block) + return unless node.respond_to?(:children) + + node.children.each do |child| + yield child if child.type == :codespan + each_codespan_in(child, &block) + end + end + def registry @registry ||= Primer::Yard::Registry.make end diff --git a/previews/primer/alpha/action_menu_preview.rb b/previews/primer/alpha/action_menu_preview.rb index 27df317504..ad9a61568c 100644 --- a/previews/primer/alpha/action_menu_preview.rb +++ b/previews/primer/alpha/action_menu_preview.rb @@ -217,8 +217,8 @@ def with_deferred_preloaded_content # @label With actions # # @param disable_items toggle - def with_actions(disable_items: false) - render_with_template(locals: { disable_items: disable_items }) + def with_actions(disable_items: false, route_format: :html) + render_with_template(locals: { disable_items: disable_items, route_format: route_format }) end # @label Single select form @@ -227,6 +227,12 @@ def single_select_form(route_format: :html) render_with_template(locals: { route_format: route_format }) end + # @label Single select form items + # + def single_select_form_items(route_format: :html) + render_with_template(locals: { route_format: route_format }) + end + # @label Multiple select form # def multiple_select_form(route_format: :html) diff --git a/previews/primer/alpha/action_menu_preview/single_select_form_items.html.erb b/previews/primer/alpha/action_menu_preview/single_select_form_items.html.erb new file mode 100644 index 0000000000..f1ad7101e1 --- /dev/null +++ b/previews/primer/alpha/action_menu_preview/single_select_form_items.html.erb @@ -0,0 +1,31 @@ +<%= render(Primer::Alpha::ActionMenu.new(select_variant: :single)) do |menu| %> + <% menu.with_show_button { "Group By" } %> + <% menu.with_item( + label: "Repository", + href: action_menu_form_action_path(format: route_format), + form_arguments: { + method: :post, + inputs: [{ + name: "query", + value: "query" + }, { + name: "foo", # use "foo" here because that's what the controller expects + value: "group-by-repository", + }], + } + ) %> + <% menu.with_item( + label: "Date", + href: action_menu_form_action_path(format: route_format), + form_arguments: { + method: :post, + inputs: [{ + name: "query", + value: "query" + }, { + name: "foo", # use "foo" here because that's what the controller expects + value: "sort-by-date" + }] + } + ) %> +<% end %> diff --git a/previews/primer/alpha/action_menu_preview/with_actions.html.erb b/previews/primer/alpha/action_menu_preview/with_actions.html.erb index 11b0f93a0b..a51b298123 100644 --- a/previews/primer/alpha/action_menu_preview/with_actions.html.erb +++ b/previews/primer/alpha/action_menu_preview/with_actions.html.erb @@ -13,7 +13,7 @@ <% component.with_item(label: "Copy text", tag: :"clipboard-copy", content_arguments: { value: "Text to copy" }, disabled: disable_items) %> <% component.with_item( label: "Submit form", - href: action_menu_form_action_path, + href: action_menu_form_action_path(format: route_format), form_arguments: { name: "foo", value: "bar", method: :post }, diff --git a/test/system/alpha/action_menu_test.rb b/test/system/alpha/action_menu_test.rb index 6d6920ede4..95c066fc3a 100644 --- a/test/system/alpha/action_menu_test.rb +++ b/test/system/alpha/action_menu_test.rb @@ -435,12 +435,61 @@ def test_multiple_select_form_uses_label_if_no_value_provided end def test_individual_items_can_submit_post_requests_via_forms - visit_preview(:with_actions) + visit_preview(:with_actions, route_format: :json) click_on_invoker_button click_on_fourth_item - assert_equal page.text, 'You selected "bar"' + response = JSON.parse(find("pre").text) + assert_equal "bar", response["value"] + end + + def test_single_select_items_can_submit_forms + visit_preview(:single_select_form_items, route_format: :json) + + click_on_invoker_button + click_on_first_item + + # for some reason the JSON response is wrapped in HTML, I have no idea why + response = JSON.parse(find("pre").text) + assert_equal "group-by-repository", response["value"] + end + + def test_single_select_items_can_submit_forms_on_enter + visit_preview(:single_select_form_items, route_format: :json) + + focus_on_invoker_button + + # open menu, "click" first item + page.driver.browser.keyboard.type(:enter, :enter) + + # for some reason the JSON response is wrapped in HTML, I have no idea why + response = JSON.parse(find("pre").text) + assert_equal "group-by-repository", response["value"] + end + + def test_single_select_items_can_submit_forms_on_keydown_space + visit_preview(:single_select_form_items, route_format: :json) + + focus_on_invoker_button + + # open menu, "click" first item + page.driver.browser.keyboard.type(:enter, :space) + + # for some reason the JSON response is wrapped in HTML, I have no idea why + response = JSON.parse(find("pre").text) + assert_equal "group-by-repository", response["value"] + end + + def test_single_select_items_can_submit_forms_with_multiple_fields + visit_preview(:single_select_form_items, route_format: :json) + + click_on_invoker_button + click_on_first_item + + # for some reason the JSON response is wrapped in HTML, I have no idea why + response = JSON.parse(find("pre").text) + assert_equal "query", response.dig("other_params", "query") end def test_deferred_loading