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

Document changes to sorting icons and icon component #812

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

maxwondercorn
Copy link
Collaborator

Document changes to icon parameters and required icon component

@RobbieTheWagner
Copy link
Member

@maxwondercorn I'm not sure if we need to make these updates? We're using the same API as before, we're just now passing components instead of classes. If someone wanted to pass just the classes, they could and they could include their own CSS to make those classes add icons.

@maxwondercorn
Copy link
Collaborator Author

@rwwagner90 I see what you're saying about being able still use classes. A case of overthinking. I think we should be less font awesome specific. Something more generic such as "class names to use with custom CSS, web fonts, or ids to use with a custom icon component"

This is related to the incomplete upgrade guide. I call out using the included font awesome icons require implementing the fa-icon-wrapper or similar component, need to add your own icon packs to use other icons, etc.

I'll get back to this

@RobbieTheWagner
Copy link
Member

@maxwondercorn yes, I agree. Perhaps we should move all the FA deps to devDeps and update the docs to be more generic, but also show a new FA example.

@maxwondercorn
Copy link
Collaborator Author

@maxwondercorn yes, I agree. Perhaps we should move all the FA deps to devDeps and update the docs to be more generic, but also show a new FA example.

I agree as that looks like the original implementation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants