From cc7d5e77d43da217a31ee7e739071cc6bce25eb3 Mon Sep 17 00:00:00 2001 From: Neal Lindsay Date: Fri, 29 Jul 2022 13:00:15 -0400 Subject: [PATCH 1/3] ClipboardCopy only needs aria-label when empty We were requiring `aria-label` on ClipboardCopy in all cases, but when the component already contains suitable text this is not recommended. This changes when we validate the presence of `aria-label`. At instantiation time the component doesn't know if it will be rendered with a block, so we check at render time instead. Fixes #1081 --- .changeset/good-coats-shout.md | 5 +++++ app/components/primer/clipboard_copy.rb | 10 ++++++++-- test/components/clipboard_copy_test.rb | 8 +++++++- 3 files changed, 20 insertions(+), 3 deletions(-) create mode 100644 .changeset/good-coats-shout.md diff --git a/.changeset/good-coats-shout.md b/.changeset/good-coats-shout.md new file mode 100644 index 0000000000..74f452df1f --- /dev/null +++ b/.changeset/good-coats-shout.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Stop requiring aria-label on ClipboardCopy with other content. diff --git a/app/components/primer/clipboard_copy.rb b/app/components/primer/clipboard_copy.rb index 547f898cd7..faeb9620c1 100644 --- a/app/components/primer/clipboard_copy.rb +++ b/app/components/primer/clipboard_copy.rb @@ -12,7 +12,7 @@ class ClipboardCopy < Primer::Component # <%= render(Primer::ClipboardCopy.new(value: "Text to copy", "aria-label": "Copy text to the system clipboard")) %> # # @example With text instead of icons - # <%= render(Primer::ClipboardCopy.new(value: "Text to copy", "aria-label": "Copy text to the system clipboard")) do %> + # <%= render(Primer::ClipboardCopy.new(value: "Text to copy")) do %> # Click to copy! # <% end %> # @@ -34,10 +34,16 @@ def initialize(value: nil, **system_arguments) @system_arguments[:value] = value if value.present? end + # :nodoc: + def render_in(*args, &block) + validate_aria_label unless block_given? + + super + end + private def validate! - validate_aria_label raise ArgumentError, "Must provide either `value` or `for`" if @value.nil? && @system_arguments[:for].nil? end end diff --git a/test/components/clipboard_copy_test.rb b/test/components/clipboard_copy_test.rb index 40019bbb31..49470bb406 100644 --- a/test/components/clipboard_copy_test.rb +++ b/test/components/clipboard_copy_test.rb @@ -14,8 +14,14 @@ def test_renders_simple end end + def test_requires_aria_label_when_empty + assert_raises(ArgumentError) do + render_inline Primer::ClipboardCopy.new(value: "my-branch-name") + end + end + def test_renders_with_text_contents - render_inline Primer::ClipboardCopy.new(value: "my-branch-name", "aria-label": "Copy branch name to clipboard") do + render_inline Primer::ClipboardCopy.new(value: "my-branch-name") do "Click to copy!" end From 0a5cb861418c6ea3a19d64ff7d156916e50a0a7b Mon Sep 17 00:00:00 2001 From: Neal Lindsay Date: Fri, 29 Jul 2022 13:43:16 -0400 Subject: [PATCH 2/3] All ClipboardCopy without content needs aria-label I was just checking for a block to decide if ClipboardCopy needed an aria-label. But @camertron suggested that checking for blank content would be better, and I think the code is nicer as well. --- app/components/primer/clipboard_copy.rb | 6 ++---- test/components/clipboard_copy_test.rb | 3 ++- 2 files changed, 4 insertions(+), 5 deletions(-) diff --git a/app/components/primer/clipboard_copy.rb b/app/components/primer/clipboard_copy.rb index faeb9620c1..c7fc087408 100644 --- a/app/components/primer/clipboard_copy.rb +++ b/app/components/primer/clipboard_copy.rb @@ -35,10 +35,8 @@ def initialize(value: nil, **system_arguments) end # :nodoc: - def render_in(*args, &block) - validate_aria_label unless block_given? - - super + def before_render + validate_aria_label if content.blank? end private diff --git a/test/components/clipboard_copy_test.rb b/test/components/clipboard_copy_test.rb index 49470bb406..4b12c5fc21 100644 --- a/test/components/clipboard_copy_test.rb +++ b/test/components/clipboard_copy_test.rb @@ -16,7 +16,8 @@ def test_renders_simple def test_requires_aria_label_when_empty assert_raises(ArgumentError) do - render_inline Primer::ClipboardCopy.new(value: "my-branch-name") + render_inline Primer::ClipboardCopy.new(value: "my-branch-name") do + end end end From 88bb2d42f1d0a5bb54634ace812b5b22febedb88 Mon Sep 17 00:00:00 2001 From: Neal Lindsay Date: Fri, 29 Jul 2022 14:54:54 -0400 Subject: [PATCH 3/3] Fill my intentionally empty block for RuboCop An explicit nil tests just as well while satisfying the linter. --- test/components/clipboard_copy_test.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/test/components/clipboard_copy_test.rb b/test/components/clipboard_copy_test.rb index 4b12c5fc21..9b2026e193 100644 --- a/test/components/clipboard_copy_test.rb +++ b/test/components/clipboard_copy_test.rb @@ -17,6 +17,7 @@ def test_renders_simple def test_requires_aria_label_when_empty assert_raises(ArgumentError) do render_inline Primer::ClipboardCopy.new(value: "my-branch-name") do + nil end end end