From 721cde112dc7f90c7f8d43e0979117fe238b6172 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Mon, 10 Jun 2024 10:57:05 -0700 Subject: [PATCH 1/4] Add a leading spinner to the TextField component --- lib/primer/forms/dsl/text_field_input.rb | 9 ++++++- lib/primer/forms/primer_text_field.ts | 29 +++++++++++++++++---- lib/primer/forms/text_field.html.erb | 8 ++++-- previews/primer/alpha/text_field_preview.rb | 15 +++++++++-- test/components/alpha/text_field_test.rb | 16 ++++++++++++ test/system/alpha/text_field_test.rb | 22 ++++++++++++++++ 6 files changed, 89 insertions(+), 10 deletions(-) diff --git a/lib/primer/forms/dsl/text_field_input.rb b/lib/primer/forms/dsl/text_field_input.rb index 3b03eb4ca3..698189fcc8 100644 --- a/lib/primer/forms/dsl/text_field_input.rb +++ b/lib/primer/forms/dsl/text_field_input.rb @@ -7,17 +7,20 @@ module Dsl class TextFieldInput < Input attr_reader( *%i[ - name label show_clear_button leading_visual clear_button_id + name label show_clear_button leading_visual leading_spinner clear_button_id visually_hide_label inset monospace field_wrap_classes auto_check_src ] ) + alias leading_spinner? leading_spinner + def initialize(name:, label:, **system_arguments) @name = name @label = label @show_clear_button = system_arguments.delete(:show_clear_button) @leading_visual = system_arguments.delete(:leading_visual) + @leading_spinner = !!system_arguments.delete(:leading_spinner) @clear_button_id = system_arguments.delete(:clear_button_id) @inset = system_arguments.delete(:inset) @monospace = system_arguments.delete(:monospace) @@ -30,6 +33,10 @@ def initialize(name:, label:, **system_arguments) ) end + if @leading_spinner && !@leading_visual + raise ArgumentError, "text fields that request a leading spinner must also specify a leading visual" + end + super(**system_arguments) add_input_data(:target, "primer-text-field.inputElement #{system_arguments.dig(:data, :target) || ''}") diff --git a/lib/primer/forms/primer_text_field.ts b/lib/primer/forms/primer_text_field.ts index ce86245933..080b451be1 100644 --- a/lib/primer/forms/primer_text_field.ts +++ b/lib/primer/forms/primer_text_field.ts @@ -1,15 +1,24 @@ +/* eslint-disable custom-elements/expose-class-on-global */ + import '@github/auto-check-element' +import type {AutoCheckErrorEvent, AutoCheckSuccessEvent} from '@github/auto-check-element' import {controller, target} from '@github/catalyst' -// eslint-disable-next-line custom-elements/expose-class-on-global +declare global { + interface HTMLElementEventMap { + 'auto-check-success': AutoCheckSuccessEvent + 'auto-check-error': AutoCheckErrorEvent + } +} @controller -// eslint-disable-next-line no-unused-vars, @typescript-eslint/no-unused-vars -class PrimerTextFieldElement extends HTMLElement { +export class PrimerTextFieldElement extends HTMLElement { @target inputElement: HTMLInputElement @target validationElement: HTMLElement @target validationMessageElement: HTMLElement @target validationSuccessIcon: HTMLElement @target validationErrorIcon: HTMLElement + @target leadingVisual: HTMLElement + @target leadingSpinner: HTMLElement #abortController: AbortController | null @@ -19,7 +28,7 @@ class PrimerTextFieldElement extends HTMLElement { this.addEventListener( 'auto-check-success', - async (event: any) => { + async (event: AutoCheckSuccessEvent) => { const message = await event.detail.response.text() if (message && message.length > 0) { this.setSuccess(message) @@ -32,7 +41,7 @@ class PrimerTextFieldElement extends HTMLElement { this.addEventListener( 'auto-check-error', - async (event: any) => { + async (event: AutoCheckErrorEvent) => { const errorMessage = await event.detail.response.text() this.setError(errorMessage) }, @@ -85,4 +94,14 @@ class PrimerTextFieldElement extends HTMLElement { this.setValidationMessage(message) this.validationElement.hidden = false } + + showLeadingSpinner(): void { + this.leadingSpinner?.removeAttribute('hidden') + this.leadingVisual?.setAttribute('hidden', '') + } + + hideLeadingSpinner(): void { + this.leadingSpinner?.setAttribute('hidden', '') + this.leadingVisual?.removeAttribute('hidden') + } } diff --git a/lib/primer/forms/text_field.html.erb b/lib/primer/forms/text_field.html.erb index 0c7a3aa89f..7b06434e76 100644 --- a/lib/primer/forms/text_field.html.erb +++ b/lib/primer/forms/text_field.html.erb @@ -1,8 +1,12 @@ <%= render(FormControl.new(input: @input, tag: :"primer-text-field")) do %> <%= content_tag(:div, **@field_wrap_arguments) do %> - <% if @input.leading_visual %> + <%# leading spinner implies a leading visual %> + <% if @input.leading_visual || @input.leading_spinner? %> - <%= render(Primer::Beta::Octicon.new(**@input.leading_visual)) %> + <%= render(Primer::Beta::Octicon.new(**@input.leading_visual, data: { target: "primer-text-field.leadingVisual" })) %> + <% if @input.leading_spinner? %> + <%= render(Primer::Beta::Spinner.new(size: :small, hidden: true, data: { target: "primer-text-field.leadingSpinner" })) %> + <% end %> <% end %> <%= render Primer::ConditionalWrapper.new(condition: @input.auto_check_src, tag: "auto-check", csrf: auto_check_authenticity_token, src: @input.auto_check_src) do %> diff --git a/previews/primer/alpha/text_field_preview.rb b/previews/primer/alpha/text_field_preview.rb index 63a91526a8..e077ff0d09 100644 --- a/previews/primer/alpha/text_field_preview.rb +++ b/previews/primer/alpha/text_field_preview.rb @@ -23,6 +23,7 @@ class TextFieldPreview < ViewComponent::Preview # @param inset toggle # @param monospace toggle # @param leading_visual_icon octicon + # @param leading_spinner toggle def playground( name: "my-text-field", id: "my-text-field", @@ -40,7 +41,8 @@ def playground( placeholder: nil, inset: false, monospace: false, - leading_visual_icon: nil + leading_visual_icon: nil, + leading_spinner: false ) system_arguments = { name: name, @@ -58,7 +60,8 @@ def playground( validation_message: validation_message, placeholder: placeholder, inset: inset, - monospace: monospace + monospace: monospace, + leading_spinner: leading_spinner } if leading_visual_icon @@ -68,6 +71,14 @@ def playground( } end + # You're required to specify a leading visual if a leading spinner is requested + if leading_spinner && !leading_visual_icon + system_arguments[:leading_visual] = { + icon: :search, + size: :small + } + end + render(Primer::Alpha::TextField.new(**system_arguments)) end diff --git a/test/components/alpha/text_field_test.rb b/test/components/alpha/text_field_test.rb index 14d5c8e2f1..b3cfc3f590 100644 --- a/test/components/alpha/text_field_test.rb +++ b/test/components/alpha/text_field_test.rb @@ -86,4 +86,20 @@ def test_renders_a_leading_visual_icon assert_selector "svg.octicon.octicon-search.FormControl-input-leadingVisual" end end + + def test_renders_a_spinner + render_inline( + Primer::Alpha::TextField.new(**@default_params, leading_visual: { icon: :search }, leading_spinner: true) + ) + + assert_selector "svg[data-target='primer-text-field.leadingSpinner']", visible: :hidden + end + + def test_enforces_leading_visual_when_spinner_requested + error = assert_raises(ArgumentError) do + render_inline(Primer::Alpha::TextField.new(**@default_params, leading_spinner: true)) + end + + assert_includes error.message, "must also specify a leading visual" + end end diff --git a/test/system/alpha/text_field_test.rb b/test/system/alpha/text_field_test.rb index 38dd1c771e..39a0b3aa66 100644 --- a/test/system/alpha/text_field_test.rb +++ b/test/system/alpha/text_field_test.rb @@ -4,6 +4,8 @@ module Alpha class IntegrationTextFieldTest < System::TestCase + include Primer::JsTestHelpers + def test_clear_button visit_preview(:show_clear_button) @@ -58,5 +60,25 @@ def test_custom_data_target assert_selector "input[data-target*='primer-text-field.inputElement']" assert_selector "input[data-target*='custom-component.inputElement']" end + + def test_show_and_hide_leading_spinner + visit_preview(:playground, leading_spinner: true) + + evaluate_multiline_script(<<~JS) + const textField = document.querySelector('primer-text-field') + textField.showLeadingSpinner() + JS + + assert_selector "[data-target='primer-text-field.leadingSpinner']" + refute_selector "[data-target='primer-text-field.leadingVisual']" + + evaluate_multiline_script(<<~JS) + const textField = document.querySelector('primer-text-field') + textField.hideLeadingSpinner() + JS + + assert_selector "[data-target='primer-text-field.leadingVisual']" + refute_selector "[data-target='primer-text-field.leadingSpinner']" + end end end From ab18bf9021972d1623da0080be83ad13b74ba614 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Mon, 10 Jun 2024 11:01:55 -0700 Subject: [PATCH 2/4] Add changeset --- .changeset/pretty-planets-drum.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/pretty-planets-drum.md diff --git a/.changeset/pretty-planets-drum.md b/.changeset/pretty-planets-drum.md new file mode 100644 index 0000000000..2a5637b6e3 --- /dev/null +++ b/.changeset/pretty-planets-drum.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': minor +--- + +Add a leading spinner to the TextField component. From 0dfd41418110c154cd21265f55f16e05b084ae67 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Mon, 10 Jun 2024 11:12:04 -0700 Subject: [PATCH 3/4] Update docs --- app/components/primer/alpha/text_field.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/components/primer/alpha/text_field.rb b/app/components/primer/alpha/text_field.rb index f50f16bdf3..3fc5590fc2 100644 --- a/app/components/primer/alpha/text_field.rb +++ b/app/components/primer/alpha/text_field.rb @@ -26,6 +26,7 @@ class TextField < Primer::Component # @param monospace [Boolean] If `true`, uses a monospace font for the input field. # @param auto_check_src [String] When provided, makes a request to the given URL whenever the contents of the text field changes. If the server responds with a non-2xx status code, the response body is used as the validation message. # @param leading_visual [Hash] Renders a leading visual icon before the text field's cursor. The hash will be passed to Primer's <%= link_to_component(Primer::Beta::Octicon) %> component. + # @param leading_spinner [Boolean] If `true`, a leading spinner will be included in the markup. The spinner can be shown via the `showLeadingSpinner()` JavaScript method, and hidden via `hideLeadingSpinner()`. If this argument is `true`, a leading visual must also be provided. # @param show_clear_button [Boolean] Whether or not to include a clear button inside the input that clears the input's contents when clicked. # @param clear_button_id [String] The HTML id attribute of the clear button. end From 35c9ee4f5db392de5269e049dbb51670a5c49282 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Mon, 10 Jun 2024 11:28:03 -0700 Subject: [PATCH 4/4] Add similar spinner test in lib/ --- test/lib/primer/forms/text_field_input_test.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/test/lib/primer/forms/text_field_input_test.rb b/test/lib/primer/forms/text_field_input_test.rb index 02644652fe..98f3c9d84c 100644 --- a/test/lib/primer/forms/text_field_input_test.rb +++ b/test/lib/primer/forms/text_field_input_test.rb @@ -46,4 +46,18 @@ def test_leading_visual assert_selector "svg.octicon.octicon-search.FormControl-input-leadingVisual" end + + def test_enforces_leading_visual_when_spinner_requested + error = assert_raises(ArgumentError) do + render_in_view_context do + primer_form_with(url: "/foo") do |f| + render_inline_form(f) do |text_field_form| + text_field_form.text_field(name: :foo, label: "Foo", leading_spinner: true) + end + end + end + end + + assert_includes error.message, "must also specify a leading visual" + end end