From 18445b1d47dc197e33899758160b7e4f6ba5d92c Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 21 Mar 2023 10:53:13 -0700 Subject: [PATCH 1/3] Modify merge_aria to combine plural attributes; introduce merge_data --- app/components/primer/component.rb | 60 ++++++++++++++++++++++++++++-- test/components/component_test.rb | 21 +++++++++-- 2 files changed, 75 insertions(+), 6 deletions(-) diff --git a/app/components/primer/component.rb b/app/components/primer/component.rb index e0127e25d3..47cd89ba9d 100644 --- a/app/components/primer/component.rb +++ b/app/components/primer/component.rb @@ -25,6 +25,8 @@ class Component < ViewComponent::Base include Audited::Dsl INVALID_ARIA_LABEL_TAGS = [:div, :span, :p].freeze + PLURAL_ARIA_ATTRIBUTES = %i[describedby labelledby].freeze + PLURAL_DATA_ATTRIBUTES = %i[target targets].freeze def self.deprecated? status == :deprecated @@ -64,6 +66,12 @@ def aria(val, system_arguments) # Eg. merge_aria({ "aria-disabled": "true" }, { aria: { invalid: "true" } }) # => { disabled: "true", invalid: "true" } # + # Certain aria attributes can contain multiple values separated by spaces. merge_aria + # will combine these plural attributes into a composite string. + # + # Eg. merge_aria({ "aria-labelledby": "foo" }, { aria: { labelledby: "bar" } }) + # => { labelledby: "foo bar" } + # # It's designed to be used to normalize and merge aria information from system_arguments # hashes. Consider using this pattern in component initializers: # @@ -72,17 +80,63 @@ def aria(val, system_arguments) # { aria: { labelled_by: id } } # ) def merge_aria(*hashes) + merge_prefixed_attribute_hashes( + *hashes, prefix: :aria, plural_keys: PLURAL_ARIA_ATTRIBUTES + ) + end + + # Merges hashes that contain "data-*" keys and nested data: hashes. Removes keys from + # each hash and returns them in the new hash. + # + # Eg. merge_data({ "data-foo": "true" }, { data: { bar: "true" } }) + # => { foo: "true", bar: "true" } + # + # Certain data attributes can contain multiple values separated by spaces. merge_data + # will combine these plural attributes into a composite string. + # + # Eg. merge_data({ "data-target": "foo" }, { data: { target: "bar" } }) + # => { target: "foo bar" } + # + # It's designed to be used to normalize and merge data information from system_arguments + # hashes. Consider using this pattern in component initializers: + # + # @system_arguments[:data] = merge_aria( + # @system_arguments, + # { data: { foo: "bar" } } + # ) + def merge_data(*hashes) + merge_prefixed_attribute_hashes( + *hashes, prefix: :data, plural_keys: PLURAL_DATA_ATTRIBUTES + ) + end + + def merge_prefixed_attribute_hashes(*hashes, prefix:, plural_keys:) {}.tap do |result| hashes.each do |hash| next unless hash - result.merge!(hash.delete(:aria) || {}) + prefix_hash = hash.delete(prefix) || {} + + prefix_hash.each_pair do |key, val| + result[key] = if plural_keys.include?(key) + [*(result[key] || "").split, val].join(" ").strip + else + val + end + end hash.delete_if do |key, val| key_s = key.to_s - if key.start_with?("aria-") - result[key_s.sub("aria-", "").to_sym] = val + if key.start_with?("#{prefix}-") + bare_key = key_s.sub("#{prefix}-", "").to_sym + + result[bare_key] = if plural_keys.include?(bare_key) + [*(result[bare_key] || "").split, val].join(" ").strip + else + val + end + true else false diff --git a/test/components/component_test.rb b/test/components/component_test.rb index 4faf0695d0..80b49c5add 100644 --- a/test/components/component_test.rb +++ b/test/components/component_test.rb @@ -217,12 +217,27 @@ def test_deny_aria_key_does_not_raise_in_production def test_merge_aria component = Primer::Component.new - hash1 = { "aria-disabled": "true", aria: { labelledby: "foo" }, foo: "foo" } - hash2 = { aria: { invalid: "true" }, "aria-label": "bar", bar: "bar" } + hash1 = { "aria-disabled": "true", aria: { labelledby: "foo", describedby: "foo" }, foo: "foo" } + hash2 = { aria: { invalid: "true", labelledby: "bar" }, "aria-label": "bar", bar: "bar", "aria-labelledby": "baz", "aria-describedby": nil } merged_arias = component.send(:merge_aria, hash1, hash2) - assert_equal merged_arias, { disabled: "true", invalid: "true", labelledby: "foo", label: "bar" } + assert_equal merged_arias, { disabled: "true", invalid: "true", labelledby: "foo bar baz", label: "bar", describedby: "foo" } + + # assert aria info removed from original hashes + assert_equal hash1, { foo: "foo" } + assert_equal hash2, { bar: "bar" } + end + + def test_merge_data + component = Primer::Component.new + + hash1 = { "data-foo": "true", data: { target: "foo" }, foo: "foo" } + hash2 = { data: { bar: "false", target: "bar" }, "data-baz": "baz", bar: "bar", "data-target": "baz" } + + merged_data = component.send(:merge_data, hash1, hash2) + + assert_equal merged_data, { foo: "true", bar: "false", target: "foo bar baz", baz: "baz" } # assert aria info removed from original hashes assert_equal hash1, { foo: "foo" } From f9d8210545f1d5a30a20fa9c55c70358e9ca9646 Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 21 Mar 2023 10:58:06 -0700 Subject: [PATCH 2/3] Add changeset --- .changeset/eight-rockets-brush.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 .changeset/eight-rockets-brush.md diff --git a/.changeset/eight-rockets-brush.md b/.changeset/eight-rockets-brush.md new file mode 100644 index 0000000000..04759da7e1 --- /dev/null +++ b/.changeset/eight-rockets-brush.md @@ -0,0 +1,5 @@ +--- +'@primer/view-components': patch +--- + +Modify merge_aria to combine plural attributes; introduce merge_data From b8c7be7d39d4f5eac71b6354c661928646dc583e Mon Sep 17 00:00:00 2001 From: Cameron Dutro Date: Tue, 21 Mar 2023 11:04:34 -0700 Subject: [PATCH 3/3] Fix linting issues --- app/components/primer/component.rb | 22 ++++++++++++---------- 1 file changed, 12 insertions(+), 10 deletions(-) diff --git a/app/components/primer/component.rb b/app/components/primer/component.rb index 47cd89ba9d..87c423a6d9 100644 --- a/app/components/primer/component.rb +++ b/app/components/primer/component.rb @@ -118,11 +118,12 @@ def merge_prefixed_attribute_hashes(*hashes, prefix:, plural_keys:) prefix_hash = hash.delete(prefix) || {} prefix_hash.each_pair do |key, val| - result[key] = if plural_keys.include?(key) - [*(result[key] || "").split, val].join(" ").strip - else - val - end + result[key] = + if plural_keys.include?(key) + [*(result[key] || "").split, val].join(" ").strip + else + val + end end hash.delete_if do |key, val| @@ -131,11 +132,12 @@ def merge_prefixed_attribute_hashes(*hashes, prefix:, plural_keys:) if key.start_with?("#{prefix}-") bare_key = key_s.sub("#{prefix}-", "").to_sym - result[bare_key] = if plural_keys.include?(bare_key) - [*(result[bare_key] || "").split, val].join(" ").strip - else - val - end + result[bare_key] = + if plural_keys.include?(bare_key) + [*(result[bare_key] || "").split, val].join(" ").strip + else + val + end true else