ClipboardCopy only needs aria-label when empty
#1244
Conversation
🦋 Changeset detectedLatest commit: 88bb2d4 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
neall
force-pushed
the
do-not-require-aria-label-on-clipboard-copy-with-content
branch
from
cc44bb0
to
ada775e
Compare
| @@ -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? | |||
There was a problem hiding this comment.
Hmm, I'm not sure checking the presence of a block is the right approach here. The component could ostensibly be passed an empty block, or someday have slots that must be defined in the block. What would you think about something like:
def before_render
validate_aria_label if content.blank?
endThere was a problem hiding this comment.
They can always give some content that is not suitable for screen readers, but explicitly checking for blank sounds good. before_render also sounds like a better place for this check than hooking into render_in as well.
neall
force-pushed
the
do-not-require-aria-label-on-clipboard-copy-with-content
branch
2 times, most recently
from
38faf06
to
c8a1fb6
Compare
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 primer#1081
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.
neall
force-pushed
the
do-not-require-aria-label-on-clipboard-copy-with-content
branch
from
c8a1fb6
to
0a5cb86
Compare
An explicit nil tests just as well while satisfying the linter.
neall
force-pushed
the
do-not-require-aria-label-on-clipboard-copy-with-content
branch
from
69d09d1
to
88bb2d4
Compare
We were requiring
aria-labelonClipboardCopyin 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