Use `@primer/octicons-react` icons #3227

FloEdelmann · 2020-06-12T19:18:27Z

I've tried to use the official @primer/octicons-react package, as suggested in #3147 (comment).

I think this is not possible, at least not for now. (Or I am not skilled enough in TypeScript / JSX / React / dom-chef.)

The default props are not applied.

In particular the size prop is missing, so all the icons are huge.

Relevant code snippets in octicons-react/dist/index.esm.js:
There is no nice way to pass props to an icon function in dom-chef, as <SomeIcon some-prop="foo"/> is compiled to createElement(SomeIcon(), {someProp: 'foo'}) instead of createElement(SomeIcon({someProp: 'foo'})).
The icons don't have specific classes, i.e. we have to add the classes manually where we need it. E.g. this selector will break:

https://github.com/sindresorhus/refined-github/blob/4dabb8ce0539222101d39c5329857dafb88327b6/source/features/toggle-files-button.css#L11-L12

Nevertheless, I wanted to share my progress, maybe someone else can figure out how to fix those issues 🤞

fregante · 2020-06-12T21:22:20Z

3. The icons don't have specific classes, i.e. we have to add the classes manually where we need it. E.g. this selector will break:

Yeah not worth it then. If we make this change, it should make things easier during development, not harder/more verbose.

Those classes are important because sometimes we just add sections and those classes just pick up the existing styles that GitHub put in place. The less CSS we have, the less we have to update when the move things by 3px.

FloEdelmann · 2020-06-14T16:26:39Z

I created two PRs in upstream repos:

Pass defaultProps to element function vadimdemedes/dom-chef#63 to support defaultProps in dom-chef → resolves problem 1
Add icon-specific className to React default props primer/octicons#453 to add icon-specific classes to the components' defaultProps → resolves problem 3

That would leave us with just problem 2, but we could still override the resulting attributes manually instead of passing custom props, like mentioned in the suggested dom-chef readme update:

<SomeIcon width={32} height={32} /> // <svg … width="32" height="32">
<SomeIcon size={32} /> // <svg … width="16" height="16" size="32">

Now let's see if those PRs get merged 😄

fregante · 2020-09-21T16:49:28Z

Thank you for working on this! I hope primer/octicons#453 can be resolved soon

fregante · 2020-11-28T01:03:46Z

Let's get this over with:

I added a webpack loader to take care of this without having to wait forever for Octicons.

Please check, speak now or forever hold your peace.

I think the last step is to make sure that the icons are tree shaken: primer/octicons#405

webpack.config.ts

yakov116

Co-authored-by: Flo Edelmann <florian-edelmann@online.de>

fregante · 2020-11-29T06:29:30Z

@yakov116 that was due to the broken regex suggested by the linter, probably

This reverts commit 8c7b457.

fregante · 2020-11-29T06:38:25Z

❯ nr build

        background.js ⏤  18 kB
  browser-polyfill.js ⏤  3.93 kB
          options.css ⏤  1.78 kB
           options.js ⏤  32.5 kB
   refined-github.css ⏤  11.6 kB (+4 B)
    refined-github.js ⏤  168 kB (+44.2 kB)
 resolve-conflicts.js ⏤  1.03 kB

[webpack-cli] Compilation finished

Given the file increase, the icons are definitely not being tree-shaken 😬

Hopefully the beautifier is enough for AMO reviewers. If not, I have to figure out which Terser compress flag actually tree shakes

fregante · 2020-11-29T07:10:01Z

❯ nr build

        background.js ⏤  17.2 kB (-806 B)
  browser-polyfill.js ⏤  3.9 kB (-34 B)
          options.css ⏤  1.78 kB
           options.js ⏤  31.7 kB (-799 B)
   refined-github.css ⏤  11.6 kB (+4 B)
    refined-github.js ⏤  110 kB (-12.9 kB)
 resolve-conflicts.js ⏤  1.04 kB (+12 B)

[webpack-cli] Compilation finished

source/features/forked-to.tsx

fregante · 2020-11-30T05:48:26Z

source/globals.d.ts

+declare module 'react' {
+	const FC = (): JSX.Element => JSX.Element;
+	const React = {FC};
+	export default React;


I think this isn't needed, right? Doesn't the module already export the correct types?

In the react module, the return type of functional components is ReactElement, not JSX.Element (which is of course not actually true for us because not dom-chef is used instead of react). So when I remove the type declaration, I get errors like this:

source/github-widgets/notice-bar.tsx:17:6 - error TS2786: 'XIcon' cannot be used as a JSX component. Its return type 'ReactElement<any, any> | null' is not a valid JSX element. Type 'ReactElement<any, any>' is not assignable to type 'Element'. 17 <XIcon/>

fregante · 2020-11-30T20:05:16Z

Yessss 🙌

yakov116 · 2020-12-18T16:53:49Z

I still have this when building local. Any idea why?

FloEdelmann · 2020-12-18T18:24:49Z

@yakov116 npm i maybe?

yakov116 · 2020-12-18T19:17:02Z

I tried everything I delete node modules, npm update etc

fregante mentioned this pull request Jul 6, 2020

Fix octicons v2 usage #3317

Closed

fregante mentioned this pull request Oct 5, 2020

Parcel test (needs extra work) #3620

Closed

4 tasks

FloEdelmann added 3 commits October 5, 2020 23:03

Install and setup @primer/octicons-react

277f47c

Import new icons

743bffb

Override inline styles where needed

21cffc4

FloEdelmann mentioned this pull request Oct 12, 2020

Add icon-specific className to React default props primer/octicons#453

Merged

fregante added 3 commits November 27, 2020 18:51

Chi fa da sé, fa per tre

34cc36e

Merge remote-tracking branch 'origin/master' into pr/3227

a86b150

Update

8e06f35

fregante marked this pull request as ready for review November 28, 2020 01:02

fregante added 2 commits November 27, 2020 19:05

Add missing dependency

24989be

Lint

5f19c02

fregante changed the title ~~Experiment with @primer/octicons-react icons~~ Use @primer/octicons-react icons Nov 28, 2020

Move decamelize to devDependencies

2136a78

fregante requested review from fregante and yakov116 November 28, 2020 01:08

FloEdelmann commented Nov 28, 2020

View reviewed changes

webpack.config.ts Show resolved Hide resolved

FloEdelmann commented Nov 28, 2020

View reviewed changes

webpack.config.ts Outdated Show resolved Hide resolved

yakov116 requested changes Nov 28, 2020

View reviewed changes

yakov116 and others added 2 commits November 28, 2020 18:49

Regenerarte package-lock

8c7b457

Fix regex

beec361

Co-authored-by: Flo Edelmann <florian-edelmann@online.de>

Revert "Regenerarte package-lock "

282d1ca

This reverts commit 8c7b457.

fregante added 2 commits November 29, 2020 00:47

Fix TODO

c1063f2

Minify a little

cf1d84b

Hopefully the beautifier is enough for AMO reviewers. If not, I have to figure out which Terser compress flag actually tree shakes

fregante approved these changes Nov 29, 2020

View reviewed changes

fregante reviewed Nov 30, 2020

View reviewed changes

source/features/forked-to.tsx Outdated Show resolved Hide resolved

fregante reviewed Nov 30, 2020

View reviewed changes

FloEdelmann added 2 commits November 30, 2020 18:54

Merge remote-tracking branch 'origin/master' into octicons-react

5f2edf4

Join import lines

7fb4985

fregante merged commit 600ecbf into refined-github:master Nov 30, 2020

FloEdelmann deleted the octicons-react branch December 1, 2020 07:36

FloEdelmann mentioned this pull request Feb 28, 2021

Meta: Simplify Octicons usage #4036

Merged

cheap-glitch mentioned this pull request Mar 15, 2021

Restore base branch name in clean-conversation-headers #4103

Merged

kidonng mentioned this pull request Mar 24, 2021

Refactor clean-conversation-headers #4152

Merged

kovsu mentioned this pull request Sep 11, 2024

clean-conversation-headers - Restore author for open PR #7803

Merged

kovsu mentioned this pull request Nov 4, 2024

clean-conversation-headers - Support new PR view #8001

Merged

SunsetTechuila mentioned this pull request Feb 8, 2026

Fix parsing of head branch of deleted repos #8958

Draft

Uh oh!

Use @primer/octicons-react icons #3227

Use @primer/octicons-react icons #3227

Uh oh!

Conversation

FloEdelmann commented Jun 12, 2020

Uh oh!

fregante commented Jun 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

FloEdelmann commented Jun 14, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante commented Sep 21, 2020

Uh oh!

fregante commented Nov 28, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yakov116 left a comment

Choose a reason for hiding this comment

Uh oh!

fregante commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fregante commented Nov 29, 2020

Uh oh!

fregante commented Nov 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

fregante Nov 30, 2020

Choose a reason for hiding this comment

Uh oh!

FloEdelmann Nov 30, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fregante commented Nov 30, 2020

Uh oh!

yakov116 commented Dec 18, 2020

Uh oh!

FloEdelmann commented Dec 18, 2020

Uh oh!

yakov116 commented Dec 18, 2020

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

Use `@primer/octicons-react` icons #3227

Use `@primer/octicons-react` icons #3227

fregante commented Jun 12, 2020 •

edited

Loading

FloEdelmann commented Jun 14, 2020 •

edited

Loading

fregante commented Nov 28, 2020 •

edited

Loading

fregante commented Nov 29, 2020 •

edited

Loading

fregante commented Nov 29, 2020 •

edited

Loading

FloEdelmann Nov 30, 2020 •

edited

Loading