search

primer / octicons

A scalable set of icons handcrafted with <3 by GitHub
https://primer.style/foundations/icons
MIT License
8.22k stars 829 forks source link

Adding multiple missing icons #983

Closed lukasoppermann closed 3 months ago

lukasoppermann commented 10 months ago

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

Changes

changeset-bot[bot] commented 10 months ago

🦋 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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago

👋🏻 @lukasoppermann Regarding the failing checks:

maximedegreve commented 10 months ago

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 commented 10 months ago

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 commented 10 months ago
  • 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.

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

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 commented 10 months ago

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 commented 9 months ago

👋🏻 @lukasoppermann Regarding the failing checks:

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 commented 9 months ago

@eliperkins I think this worked. 🙏

lukasoppermann commented 9 months ago

@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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

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 commented 9 months ago

We'll revisit this after Universe. Thanks!

github-actions[bot] commented 7 months ago

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 commented 6 months ago

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 commented 6 months ago

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

camertron commented 5 months ago

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

camertron commented 5 months ago

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

A screenshot of the octicon export tool in Figma showing two icons with errors. The error text reads 'Origin not aligned to pixels.'

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

lukasoppermann commented 5 months ago

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 commented 5 months ago

@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 👍

A screenshot of Figma marked with large red arrows showing which tab and button to click on. A screenshot of Figma showing a dialog containing a list of Octicon names and checkmarks. A large red arrow points to the only item with a red, circled exclamation point and the text 'Origin not aligned to pixels.'
lukasoppermann commented 5 months ago

@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 commented 5 months ago

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