Adding multiple missing icons

[lukasoppermann]

This will fix https://github.com/primer/octicons/issues/973, #982 and https://github.com/github/primer/issues/2597 once it is done.

Changes

• rename commit-24 to git-commit-24 to align with the 16px version
• added:
  • icons/accessibility-24.svg
  • icons/accessibility-inset-16.svg
  • icons/accessibility-inset-24.svg
  • icons/apps-24.svg
  • icons/bookmark-filled.svg
  • icons/bookmark-slash-fill-16.svg
  • icons/cache-24.svg
  • icons/chevron-left-12.svg
  • icons/device-camera-24.svg
  • icons/diff-added-24.svg
  • icons/diff-ignored-24.svg
  • icons/diff-modified-24.svg
  • icons/diff-removed-24.svg
  • icons/diff-renamed-24.svg
  • icons/ellipsis-24.svg
  • icons/file-added-24.svg
  • icons/file-badge-24.svg
  • icons/file-directory-open-fill-24.svg
  • icons/file-media-16.svg
  • icons/file-moved-24.svg
  • icons/file-removed-24.svg
  • icons/fiscal-host-24.svg
  • icons/home-fill-16.svg
  • icons/id-badge-24.svg
  • icons/key-asterisk-24.svg
  • icons/logo-gist-24.svg
  • icons/logo-github-24.svg
  • icons/mark-github-24.svg
  • icons/markdown-24.svg
  • icons/meter-24.svg
  • icons/paintbrush-24.svg
  • icons/redo-24.svg
  • icons/repo-delete-24.svg
  • icons/sliders-24.svg
  • icons/sparkle-fill-24.svg
  • icons/tab-16.svg
  • icons/tab-external-24.svg
  • icons/three-bars-24.svg
  • icons/undo-24.svg

[changeset-bot[bot]]

🦋 Changeset detected

Latest commit: 353ff1c03f805fd1ef4d6cb1042388a4670727d0

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

[gavinmn]

I'm a bit hesitant to approve until I understand why these weren't added in the first place? Some of them—like the logo marks I think may have purposefully been excluded? cc @tallys

[lukasoppermann]

I'm a bit hesitant to approve until I understand why these weren't added in the first place? Some of them—like the logo marks I think may have purposefully been excluded?

I am also not sure about this. Happy to get more insight in this. I just jumped on this due to another issue that @tallys forwarded with some missing.

I feel that we currently don't have a well-documented approach as to when an icon should be excluded from one of the two sizes.

As far as I could see in the docs it states that icons should be always added in both sizes. I feel like we should add some more context around this in the docs.

Maybe @edokoa or @maximedegreve have some knowledge on this?

[tallys]

Hi all, yes, @lukasoppermann we should look at this during a working session and look at each one - to sift through intentionally omitted (logomarks and brand icons) or accidentally (only need of one size, which I agree, is inconsistent!)

Let's add this to the agenda for the next working session and work through it there @gavinmn

[broccolinisoup]

👋🏻 @lukasoppermann Regarding the failing checks:

• I believe https://github.com/primer/octicons/actions/runs/6034055222/job/16371762629?pr=983 should be resolved when we run yarn test -u to make sure the snapshots are updated. We can do it once we clarify the questions above and finalise which svgs are going to be added in this PR.
• https://github.com/primer/octicons/actions/runs/6034055222/job/16371794609?pr=983 seems like this is failing because the expected with in the test is 32 but in the new markup-github-24's width is 33? 😵 I don't really understand these Jekkly tests. We don't have github-mark-32 file, but we have tests for it? 🤷🏻‍♀️ Is this something that I am misreading? @gavinmn do you have any historical background on these tests?
• Optimize SVGs, I have no clue why they fail and the messages are very cryptic as well 😞 I tracked down the history to see if there is anyone can help us with this and seems like @eliperkins is familiar with these files. Hello Eli 👋🏻 Is this something you can help us understand why the optimise SVG jobs are failing? 🙏🏻

[maximedegreve]

Hey there! 👋 I'm not as well-versed in our icon library as some of our team members. From what I do know, it seems like feed icons were left out of the larger versions. I'm a bit uncertain about why the other icons weren't included in larger sizes.

[gavinmn]

https://github.com/primer/octicons/actions/runs/6034055222/job/16371794609?pr=983 seems like this is failing because the expected with in the test is 32 but in the new markup-github-24's width is 33? 😵 I don't really understand these Jekkly tests. We don't have github-mark-32 file, but we have tests for it? 🤷🏻‍♀️ Is this something that I am misreading? @gavinmn do you have any historical background on these tests?

I don't really have any context on these and am equally unsure what's going on with this one...

I'm a bit uncertain about why the other icons weren't included in larger sizes.

I think this is mostly some historical design debt to clean up. The 16px icons are used far more often and there have been some instances where we didn't have 24px icons ready to ship at the same time.

[eliperkins]

• Optimize SVGs, I have no clue why they fail and the messages are very cryptic as well 😞 I tracked down the history to see if there is anyone can help us with this and seems like @eliperkins is familiar with these files. Hello Eli 👋🏻 Is this something you can help us understand why the optimise SVG jobs are failing? 🙏🏻

Taking a look at the Actions logs for Optimize SVGs, it looks like it's failing in the remove_nonsvg_content method, where it goes to access nsmap on the SVG file:

File "/opt/hostedtoolcache/Python/3.10.12/x64/lib/python3.10/site-packages/picosvg/svg.py", line 954, in remove_nonsvg_content
    if self.svg_root.nsmap[None] == svgns():

Taking a look at the SVGs introduced in this PR, it seems that none of them include the attribute xmlns="http://www.w3.org/2000/svg" within the <svg> tag at the root, which would tell consumers of these XML files that they're actually SVGs and not non-SVG content.

• https://github.com/primer/octicons/blob/ada2cd6054f80de357d34f3ebedb8fa30c82d22d/icons/accessibility-24.svg?short_path=05bc098#L1

You can see the xmlns="http://www.w3.org/2000/svg" existing in other SVGs within the repo already:

• https://github.com/primer/octicons/blob/13c66aeddc0735279f9b68b5222d3e56baa5fcb9/icons/alert-16.svg?short_path=b901c51#L1

Should we try adding the xmlns="http://www.w3.org/2000/svg" attribute to the root <svg> element within the files added in this PR and see if that appeases the optimization step?

[JonathanXDR]

Hello together,

I opened the issue #973 regarding discrepancies between the Figma Octicons set and the React library. I want to express my gratitude to @lukasoppermann for promptly addressing the issue with this PR.

It's been three weeks since this PR was opened, and from the discussion, it seems like there are still some open questions and checks that need to be addressed. I understand that the process of reviewing and merging changes, especially significant ones like this, requires time and meticulous attention. However, I'm keen to see this PR progress, as it addresses the concerns I raised in the original issue.

Is there any way I can assist in moving this forward? Perhaps with additional testing or clarifications? I would appreciate any updates on the status of this PR or an estimated timeline for its resolution.

Thank you for your hard work and dedication to maintaining the Octicons library. Looking forward to seeing this issue resolved soon.

Best regards

[lukasoppermann]

👋🏻 @lukasoppermann Regarding the failing checks:

• I believe https://github.com/primer/octicons/actions/runs/6034055222/job/16371762629?pr=983 should be resolved when we run yarn test -u to make sure the snapshots are updated. We can do it once we clarify the questions above and finalise which svgs are going to be added in this PR.

Hey @broccolinisoup, I needed to run cp -r icons lib/build/svg first. If I do, I get all green checks, but no new snapshots. What do I do now?

[lukasoppermann]

@eliperkins I think this worked. 🙏

[lukasoppermann]

@broccolinisoup so the issue with the npm test is:

@primer/octicons-react › should not update exports without a semver change

See here

But the semver increase is done via changesets, right? So obviously I didn't bump the version. Is that wrong?

[lukasoppermann]

Hey folks, what do we need to do to move this on and merge it? I don't want it to die.

CC: @broccolinisoup @gavinmn @tallys

[broccolinisoup]

Hey @lukasoppermann apologise for late response!

But the semver increase is done via changesets, right? So obviously I didn't bump the version. Is that wrong?

Yes semver is done vie changeset! It throws an error here mainly because the export tests are failing. Because we are exporting new icons that are not in the snapshots.

Could you try running yarn test -u in lib/react-octiconsfolder and commit the tests/public-api.test.js file?

[lukasoppermann]

Nevermind I got it fixed by:

1. cd lib/octicons_react
2. yarn
3. yarn build
4. yarn test -u

@broccolinisoup sadly I run into an error doing this:

yarn run v1.22.19 $ ava -v tests/*.js -u

Browserslist: caniuse-lite is outdated. Please run: npx browserslist@latest --update-db

Why you should do it regularly: https://github.com/browserslist/browserslist#browsers-data-updating /Users/lukasoppermann/GitHub/octicons/node_modules/ava/lib/worker/subprocess.js:146 throw error; ^

TypeError: process.stderr.hasColors is not a function at Object.refresh (node:internal/util/colors:12:40) at node:internal/util/colors:23:16 at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/loaders:331:7) at nativeModuleRequire (node:internal/bootstrap/loaders:362:14) at node:internal/assert/assertion_error:24:16 at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/loaders:331:7) at nativeModuleRequire (node:internal/bootstrap/loaders:362:14) at node:assert:63:24 at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/loaders:331:7) at BuiltinModule.compileForPublicLoader (node:internal/bootstrap/loaders:269:10) /Users/lukasoppermann/GitHub/octicons/node_modules/ava/lib/worker/subprocess.js:146 throw error; ^

TypeError: process.stderr.hasColors is not a function at Object.refresh (node:internal/util/colors:12:40) at node:internal/util/colors:23:16 at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/loaders:331:7) at nativeModuleRequire (node:internal/bootstrap/loaders:362:14) at node:internal/assert/assertion_error:24:16 at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/loaders:331:7) at nativeModuleRequire (node:internal/bootstrap/loaders:362:14) at node:assert:63:24 at BuiltinModule.compileForInternalLoader (node:internal/bootstrap/loaders:331:7) at BuiltinModule.compileForPublicLoader (node:internal/bootstrap/loaders:269:10) ✖ tests/build.js exited with a non-zero exit code: 1 ✖ tests/index.js exited with a non-zero exit code: 1

error Command failed with exit code 1. info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[tallys]

We'll revisit this after Universe. Thanks!

[github-actions[bot]]

Hi! This pull request has been marked as stale because it has been open with no activity for 60 days. You can comment on the pull request or remove the stale label to keep it open. If you do nothing, this pull request will be closed in 7 days.

[JonathanXDR]

Hey guys, could we reopen the PR? It kinda died since the last changes were made and I thought we might pick it up again after holidays.

CC: @tallys @lukasoppermann @gavinmn @broccolinisoup

[broccolinisoup]

I added this to the FR board to see if the next FR has capacity to take a look at the failing checks.

[camertron]

Ugh it's complaining because the LICENSE doesn't have the current year in it 😓

[camertron]

Hey @lukasoppermann, it looks like several of the icons are slightly the wrong size:

I don't have permission to edit that Figma document, could you go in, fix these, and re-export?

[lukasoppermann]

Hey @camertron,

I updated the mark-github-24 icon, but the other one is 24 for me. Where do you see the size difference? Where is this screenshot from?

[camertron]

@lukasoppermann The screenshot is from Figma. I clicked on the "Export" tab in the right-hand sidebar, then clicked the "Export Octicons" button. The checklist that pops up shows mark-github-24 is still 25x24px, but it looks like markdown-24 is good to go 👍

[lukasoppermann]

@camertron nice, I had fixed it before, as I don't use the export tool.

It should be fine now hopefully, the code looks good to me. But the checks don't complete. 😢

[camertron]

@lukasoppermann I just pushed an empty commit to get CI to run, looks like it's running now.