-
Notifications
You must be signed in to change notification settings - Fork 468
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
fix: icons in Nextjs App Router #758
base: main
Are you sure you want to change the base?
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Uh, thank you very much @motinados, this is an extremely helpful PR. Will be reviewing that this week 🔎 |
You're welcome. Happy to help. |
Hi @motinados, I see the PR is still marked as a draft. Do you think it's ready for review? |
Not yet. I still need to make changes to some components. |
🚢! |
The repair of the component that was producing errors has been completed. There are no errors in Callout and BarList, but I think changes should be made to give a sense of consistency in the user-side code. bad <TextInput icon={<BeakerIcon />} />
<Callout
title="Critical speed limit reached"
icon={ExclamationCircleIcon}
>
Turbine reached critical speed. Immediately reduce turbine speed.
</Callout> good <TextInput icon={<BeakerIcon />} />
<Callout
title="Critical speed limit reached"
icon={<ExclamationCircleIcon />}
>
Turbine reached critical speed. Immediately reduce turbine speed.
</Callout> @severinlandolt |
Hi @motinados, sorry for the delayed response – I completely missed your comment. Yes, please feel free to make any edits you deem necessary :) |
@severinlandolt |
Hey @motinados, I truly appreciate the effort you've put into fixing the Icon problem 🫶🏻 Given that this introduces a breaking change, I'll be looking at it for the v4 release, which is likely around Jan/Feb next year. |
I understand the situation, and I'm looking forward to the v4 release. |
Description
As indicated in the error message of the issue, the problem arose from passing a function from the server component to the client component. To resolve this, I made it possible to pass the Icon as a
ReactElement
.I've created a simple sample. Please take a look.
This change is related to many components that accept Icon as a Prop.
Badge, Icon, Button, MultiSelect, BaseInput, NumberInput, TextInput, SearchSelect, SearchSelectItem, Select, SelectItem, Tab
Please note that this pull request does not include tests for ReactServerComponents.
Related issue(s)
Fixes #693
What kind of change does this PR introduce? (check at least one)
Does this PR introduce a breaking change? (check one)
This pull request includes a breaking change, but the previous usage remains valid. Specifically, you can pass either the previous
ElementType
or the newReactElement
as a Prop.In the future, it might be beneficial to consider removing
ElementType
. This would help in keeping the code and documentation streamlined and simple.How has This been tested?
Screenshots (if appropriate):
The PR fulfills these requirements:
main
branch