Add `tabIndex` prop to React `Icon` component

[broccolinisoup]

👋🏼 This PR adds a new prop focusable to the Icon React component. It is a feedback came from the accessibility review. Please see the comment Add focusable="false" to SVG icons here

I don't have much knowledge around svg icons so please let me know if this is a good way of doing it or any other feedback, I would appreciate 🙏🏼

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 28989810d72a5f751a2f7b1bdc170cce69c2d9b4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package

| Name | Type | | ---------------- | ----- | | @primer/octicons | Minor |

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

[broccolinisoup]

Snapshots are failing and when I tried to update the snapshots on my local I receive below errors. Anyone has any idea?

@joshblack I have seen your recent commits for tests. Is this something you can help me with? Thank you 🙏🏼

`yarn run v1.22.19 $ jest -u FAIL tests/tree-shaking.test.js ● Console

console.warn
  '<local_path>/octicons/lib/octicons_react' is imported by virtual:__entrypoint__, but could not be resolved – treating it as an external dependency

  at Object.defaultOnWarn [as onwarn] (node_modules/rollup/dist/shared/rollup.js:558:42)
  at ModuleLoader.handleResolveId (node_modules/rollup/dist/shared/rollup.js:22490:26)
  at node_modules/rollup/dist/shared/rollup.js:22451:26

● tree shaking

'<local_path>/octicons/lib/octicons_react' is imported by virtual:__entrypoint__, but could not be resolved – treating it as an external dependency

  21 |     onwarn: ({code, message}) => {
  22 |       if (code !== 'EMPTY_BUNDLE') {
> 23 |         throw new Error(message)
     |               ^
  24 |       }
  25 |     }
  26 |   })

  at onwarn (__tests__/tree-shaking.test.js:23:15)
  at Object.onwarn (node_modules/rollup/dist/shared/rollup.js:23270:13)
  at ModuleLoader.handleResolveId (node_modules/rollup/dist/shared/rollup.js:22490:26)
  at node_modules/rollup/dist/shared/rollup.js:22451:26

› 1 snapshot updated. FAIL src/tests/octicon.js ● Test suite failed to run

Cannot find module './__generated__/icons' from 'src/index.js'

Require stack:
  src/index.js
  src/__tests__/octicon.js

  10 | }
  11 |
> 12 | export * from './__generated__/icons'
     | ^
  13 |

  at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:487:11)
  at Object.<anonymous> (src/index.js:12:1)
  at Object.<anonymous> (src/__tests__/octicon.js:4:1)

FAIL tests/public-api.test.js ● Test suite failed to run

Cannot find module '../' from '__tests__/public-api.test.js'

> 1 | import * as Octicons from '../'
    | ^
  2 |
  3 | describe('@primer/octicons-react', () => {
  4 |   it('should not update exports without a semver change', () => {

  at Resolver._throwModNotFoundError (node_modules/jest-resolve/build/resolver.js:487:11)
  at Object.<anonymous> (__tests__/public-api.test.js:1:1)

Snapshot Summary › 1 snapshot updated from 1 test suite.

Test Suites: 3 failed, 3 total Tests: 1 failed, 1 passed, 2 total Snapshots: 1 updated, 1 total Time: 1.56 s Ran all test suites.`

[broccolinisoup]

@joshblack Also, do you have any recommendation on how to fix the failing snapshots here? I mentioned the issue above.

[joshblack]

@broccolinisoup definitely! I think you can do:

# From the top-level
yarn
yarn build

cd lib/octicons_react
yarn
yarn jest -u

To update the snapshots. Sorry I missed this earlier!

[broccolinisoup]

@broccolinisoup definitely! I think you can do:

# From the top-level
yarn
yarn build

cd lib/octicons_react
yarn
yarn jest -u

To update the snapshots. Sorry I missed this earlier!

I tried this yesterday but doesn't work 😢 Same above issues. Tests are failing on the main as well due to the same "cannot find module" issues. At least on my local. I'll look into it more today and see what is going on.

[broccolinisoup]

@broccolinisoup definitely! I think you can do:

# From the top-level
yarn
yarn build

cd lib/octicons_react
yarn
yarn jest -u

To update the snapshots. Sorry I missed this earlier!

I tried this yesterday but doesn't work 😢 Same above issues. Tests are failing on the main as well due to the same "cannot find module" issues. At least on my local. I'll look into it more today and see what is going on.

All good now, sorry for the confusion 🙈 Building the octicon_react solved the unknown path issues , thank you!

[broccolinisoup]

👋🏼 @joshblack!

Thank you so much for your review! The intent of this PR was for making sure the SVG items that have aria-hidden= true, don't receive any focus by some assistive technology (here is Eric’s feedback about it) and because aria-hidden was dependent on aria-label, this is why I made the focusable attribute dependant to aria-label as well. Having said that, my solution was not inclusive enough I think in a way that I was solving the half of the problem, which is making sure SVG items don’t get focus when they have aria-hidden=true, however, the solution doesn’t help if an SVG item needs to be focused. I know it is kind of out of the “scope” of the case I am working on but while I am already here why not implementing that as well! And your comments make so much sense! I have implemented it and pushed my commit. Please let me know if there is anything I am missing. I appreciate your review so much 🙏🏼

[joshblack]

Looks great @broccolinisoup! Thanks so much for taking this on 🙏

[broccolinisoup]

Looks great @broccolinisoup! Thanks so much for taking this on 🙏

Thanks for your review @joshblack!

[broccolinisoup]

Seeking a review from @primer/octicons-reviewers 👀 🙏🏼