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

Improve custom icon support #2335

Merged
merged 4 commits into from Nov 6, 2021
Merged

Conversation

amartyn-everlaw
Copy link
Contributor

Buttons already accept custom icons, but most other components do not. This changes allows all components that have icon props to use custom icons, similar to Buttons.

The main changes are

  • Adding a generic icon type in typescript to avoid repetitive code
  • Adding an IconUtils class that has a method getJSXIcon. This method is based on renderIcon in the Button class. For strings, it will create a span with the string as the className. For React.ReactNode objects or functions that return a React.ReactNode object, it will wrap the object in a span and return it
  • Updating components to use getJSXIcon instead of creating span or i icons directly

I don't know if this will be merged, but I thought it might useful to other people who want to use custom icons

@amartyn-everlaw
Copy link
Contributor Author

@mertsincan Any thoughts on this?

@mertsincan
Copy link
Member

Hi @amymartyn,

Nice PR! Can you update the documentation and codes according to conflicts?

Best Regards,

@mertsincan mertsincan added the Resolution: Needs Revision The pull request can't be merged. Conflicts need to be corrected or documentation and code updated. label Nov 3, 2021
@amartyn-everlaw
Copy link
Contributor Author

@mertsincan OK, conflicts have been fixed

@mertsincan mertsincan removed the Resolution: Needs Revision The pull request can't be merged. Conflicts need to be corrected or documentation and code updated. label Nov 6, 2021
@mertsincan mertsincan merged commit a42c765 into primefaces:master Nov 6, 2021
@mertsincan
Copy link
Member

Thanks a lot for your effort, @amymartyn ;) Good idea, Nice PR ;)

@DC2009 DC2009 mentioned this pull request Jan 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants