Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use react-fontawesome instead of using fontawesome directly #2937

Merged
merged 14 commits into from Dec 2, 2019

Conversation

@tacigar
Copy link
Member

tacigar commented Nov 30, 2019

Before, Zipkin Lens used Font-Awesome directly, but I think this way has the following problems.

  • Additional configs are required for Webpack (see:
    {
    test: /webfonts\/.*\.(png|jpg|jpeg|gif|svg|woff|woff2|ttf|eot)(\?.*$|$)/,
    loader: 'file-loader',
    },
    )
  • All icons need to be imported, so bundle size will increase (see:
    $fa-font-path: "~@fortawesome/fontawesome-free/webfonts";
    @import '~@fortawesome/fontawesome-free/scss/fontawesome.scss';
    @import '~@fortawesome/fontawesome-free/scss/brands.scss';
    @import '~@fortawesome/fontawesome-free/scss/solid.scss';
    )

I think it is better to use Font-Awesome through React components provided by Fort-Awesome (https://github.com/FortAwesome/react-fontawesome).
This library seems to avoid such problems.

Copy link
Contributor

anuraaga left a comment

Yes!! Thanks a lot for this. I wasn't expecting big styling changes for this though. Is it possible to just migrate libraries with less code change? If not no worries will take a deeper look

@tacigar

This comment has been minimized.

Copy link
Member Author

tacigar commented Nov 30, 2019

Sorry for big changes.
In addition to library migration, I changed the Sidebar code a bit.
But all other changes are library migrations.

The SidebarMenuItem component received a FontAwesome icon string as props.

<SidebarMenuItem isExternal title="Zipkin Home" links={['https://zipkin.io/']} logo="fas fa-home" />
<SidebarMenuItem isExternal title="Repository" links={['https://github.com/openzipkin/zipkin']} logo="fab fa-github" />
<SidebarMenuItem isExternal title="Twitter" links={['https://twitter.com/zipkinproject']} logo="fab fa-twitter" />
<SidebarMenuItem isExternal title="Gitter" links={['https://gitter.im/openzipkin/zipkin/']} logo="fab fa-gitter" />

So it was necessary to change SidebarMenuItem's props when migrating the library.
When making that change, I realized that SidebarMenuIcon has too verbose and meaningless logic.
So I refactored the Sidebar code a bit.

If possible, I would appreciate it if you could review this change within this PR... 😭
Sorry...

overflow="auto"
>
{children}
const Layout = ({ children }) => {

This comment has been minimized.

Copy link
@tacigar

tacigar Nov 30, 2019

Author Member

I have integrated Sidebar component into Layout component, since they are very simple components,


const drawerWidth = '3.2rem';

const useStyles = makeStyles(theme => ({

This comment has been minimized.

Copy link
@tacigar

tacigar Nov 30, 2019

Author Member

This style is almost the same as the previous one.

This comment has been minimized.

Copy link
@tacigar

tacigar Nov 30, 2019

Author Member

This combines the styles of the previous Sidebar component and the previous Layout component.

return (
<Box className={classes.root}>
<CssBaseline />
<Drawer open variant="permanent" className={classes.drawer} classes={{ paper: classes.drawerPaper }}>

This comment has been minimized.

Copy link
@tacigar

tacigar Nov 30, 2019

Author Member

This Drawer was previously in Sidebar component.

@@ -1,62 +0,0 @@
/*

This comment has been minimized.

Copy link
@tacigar

tacigar Nov 30, 2019

Author Member

I thought these unit tests were meaningless and would prevent us from changing structures of the DOM tree in the future.
So I deleted these unit tests.

This comment has been minimized.

Copy link
@abesto

abesto Dec 1, 2019

Member

Was gonna ask about this, thanks for explaining your reasoning.

},
}));

export const SidebarMenuImpl = ({

This comment has been minimized.

Copy link
@tacigar

tacigar Nov 30, 2019

Author Member

I renamed SidebarMenuItem to SidebarMenu.

Copy link
Contributor

anuraaga left a comment

Thanks!

@abesto
abesto approved these changes Dec 1, 2019
Copy link
Member

abesto left a comment

+1 for more clearly separated changes, but the individual commits in here are actually doing an OK job of that I think. Methinks this is a good change 👍

@tacigar tacigar merged commit aaafc15 into openzipkin:master Dec 2, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@tacigar tacigar deleted the tacigar:tacigar/use-react-fontawesome branch Dec 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.