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

[docs] Improve Font Awesome integration #22496

Merged

Conversation

chrislambe
Copy link
Contributor

@chrislambe chrislambe commented Sep 5, 2020

As suggested in #17754 (comment).

Wanted to point out a couple things to focus on:

  • I had to add three packages to the docs workspace (@fortawesome/{fontawesome-svg-core,free-solid-svg-icons, react-fontawesome}) following the precedent set in the TextField docs: https://material-ui.com/components/text-fields/#integration-with-3rd-party-input-libraries

  • My FontAwesomeSvgIconDemo.tsx won't compile as written. It seems to be due to the declaration of the FontAwesomeSvgIcon component in the module. Here's the error:

    Compiler error
    yarn docs:typescript
    yarn run v1.22.4
    $ yarn workspace docs typescript:transpile:dev
    $ node scripts/formattedTSDemos --watch
    Something went wrong transpiling /Volumes/Storage/Documents/Projects/material-ui/docs/src/pages/components/icons/FontAwesomeSvgIconDemo.tsx
    Error: No types found
        at checkSymbol (/Volumes/Storage/Documents/Projects/material-ui/packages/typescript-to-proptypes/build/parser.js:248:19)
        at /Volumes/Storage/Documents/Projects/material-ui/packages/typescript-to-proptypes/build/parser.js:192:48
        at Array.map (<anonymous>)
        at checkType (/Volumes/Storage/Documents/Projects/material-ui/packages/typescript-to-proptypes/build/parser.js:190:63)
        at checkSymbol (/Volumes/Storage/Documents/Projects/material-ui/packages/typescript-to-proptypes/build/parser.js:262:26)
        at /Volumes/Storage/Documents/Projects/material-ui/packages/typescript-to-proptypes/build/parser.js:192:48
        at Array.map (<anonymous>)
        at checkType (/Volumes/Storage/Documents/Projects/material-ui/packages/typescript-to-proptypes/build/parser.js:190:63)
        at checkSymbol (/Volumes/Storage/Documents/Projects/material-ui/packages/typescript-to-proptypes/build/parser.js:262:26)
        at /Volumes/Storage/Documents/Projects/material-ui/packages/typescript-to-proptypes/build/parser.js:274:77
    
    ------ Summary ------
    0 demo(s) were successfully transpiled
    547 demo(s) were skipped
    1 demo(s) were unsuccessful
    
    Watching for file changes...
    

    I was able to work around this by creating the FontAwesomeSvgIcon component in the FontAwesomeSvgIconDemo component (via useMemo) but that caused a failure in the linter about prop types, so I reverted to declaring the component at the module root. Interestingly, if yarn docs:typescript was running and had already successfully compiled the demo once, it didn't have a problem, which is how I was able to complete the demo in the first place.

Otherwise I think it's ready for review!

Closes #17754

@chrislambe chrislambe marked this pull request as draft September 5, 2020 21:24
@chrislambe chrislambe marked this pull request as ready for review September 5, 2020 21:28
@mui-pr-bot
Copy link

mui-pr-bot commented Sep 5, 2020

Details of bundle changes

Generated by 🚫 dangerJS against a85f753

@mbrookes mbrookes added the docs Improvements or additions to the documentation label Sep 5, 2020
@oliviertassinari oliviertassinari changed the title [docs] Add example of a custom Font Awesome SvgIcon (#17754) [docs] Improve Font Awesome integration Sep 7, 2020
<Icon className="fa fa-plus-circle" color="secondary" />
<Icon className="fa fa-plus-circle" style={{ color: green[500] }} />
<Icon className="fa fa-plus-circle" fontSize="small" />
<Icon className="fa fa-plus-circle" style={{ fontSize: 30 }} />
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fa is deprecated

@@ -7,20 +7,21 @@ import Icon from '@material-ui/core/Icon';
const useStyles = makeStyles((theme: Theme) =>
createStyles({
root: {
'& > .fa': {
'& > *': {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generic selector

@@ -217,8 +216,7 @@ export const styles = (theme) => {
deleteIcon: {
WebkitTapHighlightColor: 'transparent',
color: deleteIconColor,
height: 22,
width: 22,
fontSize: 22,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needed to support font icons

@oliviertassinari oliviertassinari merged commit 394f214 into mui:next Sep 8, 2020
@oliviertassinari
Copy link
Member

@chrislambe Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improve Font Awesome support
4 participants