Font Awesome added#26
Conversation
mattxwang
left a comment
mattxwang
left a comment
There was a problem hiding this comment.
Hey Utsav,
This PR failed the Travis build (our CI that auto-builds the project) - specifically, you're declaring a variable you're not using:
./src/components/Editor/components/ProfilePanel.js
Line 10: 'FontAwesomeIcon' is defined but never used no-unused-vars
I'll also double-check your FA implementation later, but see if you can fix this PR so it doesn't break Travis. You can double-check it by running npm run build with no errors or warnings.
connorbo97
left a comment
connorbo97
left a comment
There was a problem hiding this comment.
Left some comments, needs some tweakin
| cursor: pointer; | ||
| flex: 0 1 auto; | ||
| width: 20px; | ||
| transition: 1s; |
There was a problem hiding this comment.
what does this transition do?
There was a problem hiding this comment.
Transition was originally added when the bars turned slowly into a cross. But now I have seen the result after removing the transition and since the transition is unnoticeable, I'll remove this.
There was a problem hiding this comment.
Big thing: please explicitly import each icon rather than building a library. It should be much better for performance. It is slightly more tedious, but it should help out a lot more.
Check up my and Connor's suggested changes. I think in the future we should phase out the other icons we have on the site and use just FA5, which should in theory be better for performance.
As for @connorbo97 's concern about performance, it's pretty valid - we're adding essentially a non-trivial amount of kb to our web app to have nice looking icons. I do think that the typographical modifiers that we'll gain by having icon fonts will benefit us in the long run, especially as we create the sketches page, add more components, etc. Worst case scenario, we can always "eject" our of FA5 if needed with only a bit of work.
|
@connorbo97 the idea is to use tree-shaking - take a look at the article, though I'm not sure how much we'd like to rework / if we need to eject. |
|
Is tree-shaking enabled in the production build just by including Rollup.js? Till then, I'll use deep-imports to help reduce the size of Font Awesome package. |
|
We don't need to add any more things to the build haha, I believe that we'll have generic optimization by just importing what we need with explicit importing. |
|
A took a look at the stable build and I feel it would be user-eye-friendly if on hovering overt the panel buttons, the cursor did not change to a edit-bar which is indistinguishable from the button background. |
mattxwang
left a comment
mattxwang
left a comment
There was a problem hiding this comment.
Okay Utsav, this looks better. In the future, I'd recommend that you avoid mixing multiple changes into one PR (i.e. the cursor hover stuff) so that we can better see what changes have been made and when. (also I imagine that it'd be better if the cursor changed to a button hover, rather than the default. We can fix this later).
3 participants
As instructed on Font Awesome page, the Font Awesome package has been added.
The Hamburger icon is in 🍔! It changes to panel collapse panel when the panel is wide open.
The panel close button inside the panel is removed.