From 9aae31a8bfd4679a81e7a7582b8eb82eb4868f10 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 5 Oct 2022 07:11:36 +1000 Subject: [PATCH 1/7] add focusable prop to svg --- lib/octicons_react/src/createIconComponent.js | 3 ++- lib/octicons_react/src/index.d.ts | 1 + 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/octicons_react/src/createIconComponent.js b/lib/octicons_react/src/createIconComponent.js index 7f08ecb86..9b474f5b5 100644 --- a/lib/octicons_react/src/createIconComponent.js +++ b/lib/octicons_react/src/createIconComponent.js @@ -10,7 +10,7 @@ export function createIconComponent(name, defaultClassName, getSVGData) { const svgDataByHeight = getSVGData() const heights = Object.keys(svgDataByHeight) - function Icon({'aria-label': ariaLabel, className, fill = 'currentColor', size, verticalAlign}) { + function Icon({'aria-label': ariaLabel, focusable = false, className, fill = 'currentColor', size, verticalAlign}) { const height = sizeMap[size] || size const naturalHeight = closestNaturalHeight(heights, height) const naturalWidth = svgDataByHeight[naturalHeight].width @@ -20,6 +20,7 @@ export function createIconComponent(name, defaultClassName, getSVGData) { return ( className?: string fill?: string From 8e0204af30a762f57a545a6b978a37ed2136fb6d Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Wed, 5 Oct 2022 08:34:01 +1000 Subject: [PATCH 2/7] add changeset and write tests --- .changeset/sixty-nails-juggle.md | 5 +++++ lib/octicons_react/src/__tests__/octicon.js | 17 +++++++++++++++++ 2 files changed, 22 insertions(+) create mode 100644 .changeset/sixty-nails-juggle.md diff --git a/.changeset/sixty-nails-juggle.md b/.changeset/sixty-nails-juggle.md new file mode 100644 index 000000000..a4d3c67ed --- /dev/null +++ b/.changeset/sixty-nails-juggle.md @@ -0,0 +1,5 @@ +--- +'@primer/octicons': minor +--- + +Add focusable prop to react icons diff --git a/lib/octicons_react/src/__tests__/octicon.js b/lib/octicons_react/src/__tests__/octicon.js index d2cdc044d..77a808c6d 100644 --- a/lib/octicons_react/src/__tests__/octicon.js +++ b/lib/octicons_react/src/__tests__/octicon.js @@ -69,6 +69,23 @@ describe('An icon component', () => { expect(container.querySelector('svg')).toHaveAttribute('aria-label', 'icon') }) + it('respects the focusable prop', () => { + const {container} = render() + expect(container.querySelector('svg')).toHaveAttribute('focusable', true) + }) + + it('sets focusable false if ariaLabel prop is not present', () => { + const {container} = render() + expect(container.querySelector('svg')).toHaveAttribute('aria-hidden', 'true') + expect(container.querySelector('svg')).toHaveAttribute('focusable', 'false') + }) + + it('sets focusable prop to given value if ariaLabel prop is present', () => { + const {container} = render() + expect(container.querySelector('svg')).toHaveAttribute('aria-hidden', 'false') + expect(container.querySelector('svg')).toHaveAttribute('focusable', 'auto') + }) + it('respects the className prop', () => { const {container} = render() expect(container.querySelector('svg')).toHaveAttribute('class', 'foo') From 7089450b1bbbe21e142a9fcc6b306a72c2a8bca8 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Thu, 6 Oct 2022 21:10:23 +1000 Subject: [PATCH 3/7] update snapshots and tests and add docs --- docs/content/packages/react.mdx | 29 +++++++++++++++++-- .../__tests__/tree-shaking.test.js | 2 +- .../__tests__/__snapshots__/octicon.js.snap | 1 + lib/octicons_react/src/__tests__/octicon.js | 11 ++----- 4 files changed, 30 insertions(+), 13 deletions(-) diff --git a/docs/content/packages/react.mdx b/docs/content/packages/react.mdx index c0a43eb23..3eb3c24ba 100644 --- a/docs/content/packages/react.mdx +++ b/docs/content/packages/react.mdx @@ -66,6 +66,23 @@ export default () => ( ) ``` +### `focusable` + +You can add the `focusable` attribute to the SVG element via the `focusable` prop, which can be either `true`, `false`, or `auto`. +If there is no `aria-label` prop presents, `focusable` prop will be set to false and this helps prevent the decorative +SVG from being announced by some specialized assistive technology browsing modes which get delayed while trying to parse SVG's markup + +```js +// Example usage +import {PlusIcon} from '@primer/octicons-react' + +export default () => ( + +) +``` + ### Sizes The `size` prop takes `small`, `medium`, and `large` values that can be used to @@ -110,10 +127,16 @@ export default () => ( ### `Octicon` (DEPRECATED) > ⚠️ The `Octicon` component is deprecated. Use icon components on their own instead: +> > ```diff +> +> ``` + - -+ -``` + +* + +```` The `Octicon` component is wrapper that passes props to its icon component. To render a specific icon, you can pass it either via the `icon` prop, or as the only child: @@ -121,7 +144,7 @@ can pass it either via the `icon` prop, or as the only child: ```jsx -``` +```` [octicons]: https://primer.style/octicons/ [primer]: https://github.com/primer/primer diff --git a/lib/octicons_react/__tests__/tree-shaking.test.js b/lib/octicons_react/__tests__/tree-shaking.test.js index 0d1b8d2f3..628756afc 100644 --- a/lib/octicons_react/__tests__/tree-shaking.test.js +++ b/lib/octicons_react/__tests__/tree-shaking.test.js @@ -50,5 +50,5 @@ test('tree shaking single export', async () => { }) const bundleSize = Buffer.byteLength(output[0].code.trim()) / 1000 - expect(`${bundleSize}kB`).toMatchInlineSnapshot(`"2.484kB"`) + expect(`${bundleSize}kB`).toMatchInlineSnapshot(`"2.645kB"`) }) diff --git a/lib/octicons_react/src/__tests__/__snapshots__/octicon.js.snap b/lib/octicons_react/src/__tests__/__snapshots__/octicon.js.snap index bf7fb20e3..74fd31a01 100644 --- a/lib/octicons_react/src/__tests__/__snapshots__/octicon.js.snap +++ b/lib/octicons_react/src/__tests__/__snapshots__/octicon.js.snap @@ -5,6 +5,7 @@ exports[`An icon component matches snapshot 1`] = ` aria-hidden="true" class="octicon octicon-alert" fill="currentColor" + focusable="false" height="16" role="img" style="display: inline-block; user-select: none; vertical-align: text-bottom; overflow: visible;" diff --git a/lib/octicons_react/src/__tests__/octicon.js b/lib/octicons_react/src/__tests__/octicon.js index 77a808c6d..2804816a3 100644 --- a/lib/octicons_react/src/__tests__/octicon.js +++ b/lib/octicons_react/src/__tests__/octicon.js @@ -69,20 +69,13 @@ describe('An icon component', () => { expect(container.querySelector('svg')).toHaveAttribute('aria-label', 'icon') }) - it('respects the focusable prop', () => { - const {container} = render() - expect(container.querySelector('svg')).toHaveAttribute('focusable', true) - }) - - it('sets focusable false if ariaLabel prop is not present', () => { - const {container} = render() - expect(container.querySelector('svg')).toHaveAttribute('aria-hidden', 'true') + it('set the focusable prop to false if ariaLabel prop is not present', () => { + const {container} = render() expect(container.querySelector('svg')).toHaveAttribute('focusable', 'false') }) it('sets focusable prop to given value if ariaLabel prop is present', () => { const {container} = render() - expect(container.querySelector('svg')).toHaveAttribute('aria-hidden', 'false') expect(container.querySelector('svg')).toHaveAttribute('focusable', 'auto') }) From 230deec009b261173f886696e7d0e911a45622c5 Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 7 Oct 2022 15:22:48 +1000 Subject: [PATCH 4/7] tabIndex prop introduced --- docs/content/packages/react.mdx | 18 +++++++++++------- lib/octicons_react/src/__tests__/octicon.js | 17 ++++++++++++----- lib/octicons_react/src/createIconComponent.js | 5 +++-- 3 files changed, 26 insertions(+), 14 deletions(-) diff --git a/docs/content/packages/react.mdx b/docs/content/packages/react.mdx index 3eb3c24ba..f036d5c07 100644 --- a/docs/content/packages/react.mdx +++ b/docs/content/packages/react.mdx @@ -66,20 +66,24 @@ export default () => ( ) ``` -### `focusable` +### `tabIndex` -You can add the `focusable` attribute to the SVG element via the `focusable` prop, which can be either `true`, `false`, or `auto`. -If there is no `aria-label` prop presents, `focusable` prop will be set to false and this helps prevent the decorative -SVG from being announced by some specialized assistive technology browsing modes which get delayed while trying to parse SVG's markup +You can add the `tabindex` attribute to an SVG element via the `tabIndex` prop if the SVG element is intended to be interactive. +`tabIndex` prop also controls the `focusable` attribute of the SVG element which is defined by SVG Tiny 1.2 and only implemented in +Internet Explorer and Microsoft Edge. + +If there is no `tabIndex` prop is present (default behaviour), it will set `focusable` attribute to `false` and this is particularly helpful +for preventing the decorative SVG from being announced by some specialized assistive technology browsing modes which can get delayed +while trying to parse the SVG markup. ```js // Example usage import {PlusIcon} from '@primer/octicons-react' export default () => ( - + + New Item + ) ``` diff --git a/lib/octicons_react/src/__tests__/octicon.js b/lib/octicons_react/src/__tests__/octicon.js index 2804816a3..642d645e4 100644 --- a/lib/octicons_react/src/__tests__/octicon.js +++ b/lib/octicons_react/src/__tests__/octicon.js @@ -69,14 +69,21 @@ describe('An icon component', () => { expect(container.querySelector('svg')).toHaveAttribute('aria-label', 'icon') }) - it('set the focusable prop to false if ariaLabel prop is not present', () => { - const {container} = render() + it('set the focusable prop to false if tabIndex prop is not present', () => { + const {container} = render() expect(container.querySelector('svg')).toHaveAttribute('focusable', 'false') }) - it('sets focusable prop to given value if ariaLabel prop is present', () => { - const {container} = render() - expect(container.querySelector('svg')).toHaveAttribute('focusable', 'auto') + it('sets focusable prop to true if tabIndex prop is present and greater than 0', () => { + const {container} = render() + expect(container.querySelector('svg')).toHaveAttribute('tabindex', '0') + expect(container.querySelector('svg')).toHaveAttribute('focusable', 'true') + }) + + it('sets focusable prop to false if tabIndex prop is -1', () => { + const {container} = render() + expect(container.querySelector('svg')).toHaveAttribute('tabindex', '-1') + expect(container.querySelector('svg')).toHaveAttribute('focusable', 'false') }) it('respects the className prop', () => { diff --git a/lib/octicons_react/src/createIconComponent.js b/lib/octicons_react/src/createIconComponent.js index 9b474f5b5..75849f0c3 100644 --- a/lib/octicons_react/src/createIconComponent.js +++ b/lib/octicons_react/src/createIconComponent.js @@ -10,7 +10,7 @@ export function createIconComponent(name, defaultClassName, getSVGData) { const svgDataByHeight = getSVGData() const heights = Object.keys(svgDataByHeight) - function Icon({'aria-label': ariaLabel, focusable = false, className, fill = 'currentColor', size, verticalAlign}) { + function Icon({'aria-label': ariaLabel, tabIndex, className, fill = 'currentColor', size, verticalAlign}) { const height = sizeMap[size] || size const naturalHeight = closestNaturalHeight(heights, height) const naturalWidth = svgDataByHeight[naturalHeight].width @@ -20,7 +20,8 @@ export function createIconComponent(name, defaultClassName, getSVGData) { return ( = 0 ? 'true' : 'false'} aria-label={ariaLabel} role="img" className={className} From 9c0871eab598a87014dd8dd372923e4828f7406e Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 7 Oct 2022 15:29:02 +1000 Subject: [PATCH 5/7] update tree shaking snapshot --- lib/octicons_react/__tests__/tree-shaking.test.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/octicons_react/__tests__/tree-shaking.test.js b/lib/octicons_react/__tests__/tree-shaking.test.js index 628756afc..b4c2bdec3 100644 --- a/lib/octicons_react/__tests__/tree-shaking.test.js +++ b/lib/octicons_react/__tests__/tree-shaking.test.js @@ -50,5 +50,5 @@ test('tree shaking single export', async () => { }) const bundleSize = Buffer.byteLength(output[0].code.trim()) / 1000 - expect(`${bundleSize}kB`).toMatchInlineSnapshot(`"2.645kB"`) + expect(`${bundleSize}kB`).toMatchInlineSnapshot(`"2.595kB"`) }) From 4449ebfaebf953c20b13fa8d88de35004ac86b2c Mon Sep 17 00:00:00 2001 From: Armagan Ersoz Date: Fri, 7 Oct 2022 15:43:58 +1000 Subject: [PATCH 6/7] changeset update & remove type --- .changeset/sixty-nails-juggle.md | 2 +- docs/content/packages/react.mdx | 15 +++------------ lib/octicons_react/src/index.d.ts | 2 +- 3 files changed, 5 insertions(+), 14 deletions(-) diff --git a/.changeset/sixty-nails-juggle.md b/.changeset/sixty-nails-juggle.md index a4d3c67ed..4a1bf13ea 100644 --- a/.changeset/sixty-nails-juggle.md +++ b/.changeset/sixty-nails-juggle.md @@ -2,4 +2,4 @@ '@primer/octicons': minor --- -Add focusable prop to react icons +Add tabIndex prop to react icons diff --git a/docs/content/packages/react.mdx b/docs/content/packages/react.mdx index f036d5c07..26c0696a0 100644 --- a/docs/content/packages/react.mdx +++ b/docs/content/packages/react.mdx @@ -79,11 +79,8 @@ while trying to parse the SVG markup. ```js // Example usage import {PlusIcon} from '@primer/octicons-react' - export default () => ( - New Item - ) ``` @@ -131,16 +128,10 @@ export default () => ( ### `Octicon` (DEPRECATED) > ⚠️ The `Octicon` component is deprecated. Use icon components on their own instead: -> > ```diff -> -> ``` - - - -* - -```` ++ +``` The `Octicon` component is wrapper that passes props to its icon component. To render a specific icon, you can pass it either via the `icon` prop, or as the only child: @@ -148,7 +139,7 @@ can pass it either via the `icon` prop, or as the only child: ```jsx -```` +``` [octicons]: https://primer.style/octicons/ [primer]: https://github.com/primer/primer diff --git a/lib/octicons_react/src/index.d.ts b/lib/octicons_react/src/index.d.ts index 46a20ebd7..23012e42e 100644 --- a/lib/octicons_react/src/index.d.ts +++ b/lib/octicons_react/src/index.d.ts @@ -6,7 +6,7 @@ type Size = 'small' | 'medium' | 'large' export interface OcticonProps { 'aria-label'?: string - focusable?: boolean | 'auto' + tabIndex?: number children?: React.ReactElement className?: string fill?: string From 28989810d72a5f751a2f7b1bdc170cce69c2d9b4 Mon Sep 17 00:00:00 2001 From: Cole Bemis Date: Fri, 7 Oct 2022 15:55:57 -0700 Subject: [PATCH 7/7] Minor copy edits --- .changeset/sixty-nails-juggle.md | 2 +- docs/content/packages/react.mdx | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.changeset/sixty-nails-juggle.md b/.changeset/sixty-nails-juggle.md index 4a1bf13ea..e3835d632 100644 --- a/.changeset/sixty-nails-juggle.md +++ b/.changeset/sixty-nails-juggle.md @@ -2,4 +2,4 @@ '@primer/octicons': minor --- -Add tabIndex prop to react icons +Add `tabIndex` prop to React icon components diff --git a/docs/content/packages/react.mdx b/docs/content/packages/react.mdx index 8d878304e..d6d220bcb 100644 --- a/docs/content/packages/react.mdx +++ b/docs/content/packages/react.mdx @@ -72,7 +72,7 @@ You can add the `tabindex` attribute to an SVG element via the `tabIndex` prop i `tabIndex` prop also controls the `focusable` attribute of the SVG element which is defined by SVG Tiny 1.2 and only implemented in Internet Explorer and Microsoft Edge. -If there is no `tabIndex` prop is present (default behaviour), it will set `focusable` attribute to `false` and this is particularly helpful +If there is no `tabIndex` prop is present (default behavior), it will set the `focusable` attribute to `false`. This is helpful for preventing the decorative SVG from being announced by some specialized assistive technology browsing modes which can get delayed while trying to parse the SVG markup.