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

fix: icons in Nextjs App Router #758

Open
wants to merge 21 commits into
base: main
Choose a base branch
from

Conversation

motinados
Copy link
Contributor

@motinados motinados commented Oct 14, 2023

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)

  • Bug fix (non-breaking change which fixes an issue)
  • New Feature (non-breaking change which adds functionality)
  • Refactor
  • Build-related changes
  • Other, please describe:

Does this PR introduce a breaking change? (check one)

  • Yes
  • No

This pull request includes a breaking change, but the previous usage remains valid. Specifically, you can pass either the previous ElementType or the new ReactElement 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:

  • It's submitted to the main branch
  • When resolving a specific issue, it's referenced in the related issue section above
  • My change requires a change to the documentation. (Managed by Tremor Team)
  • I have added tests to cover my changes
  • Check the "Allow edits from maintainers" option while creating your PR.
  • Add refs #XXX or fixes #XXX to the related issue section if your PR refers to or fixes an issue.
  • By contributing to Tremor, you confirm that you have read and agreed to Tremor's CONTRIBUTING.md guideline. You also agree that your contributions will be licensed under the Apache License 2.0 license.

@vercel
Copy link

vercel bot commented Oct 14, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
tremor-storybook ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 26, 2024 9:18pm

@severinlandolt severinlandolt self-assigned this Oct 15, 2023
@severinlandolt severinlandolt added the PR: In Review This PR is in the process of being reviewed by the team label Oct 15, 2023
@severinlandolt
Copy link
Member

Uh, thank you very much @motinados, this is an extremely helpful PR. Will be reviewing that this week 🔎

@motinados
Copy link
Contributor Author

You're welcome. Happy to help.

@severinlandolt
Copy link
Member

Hi @motinados, I see the PR is still marked as a draft. Do you think it's ready for review?

@severinlandolt severinlandolt added the PR: Breaking Change This PR contains a breaking change label Oct 18, 2023
@motinados
Copy link
Contributor Author

Not yet. I still need to make changes to some components.

@severinlandolt
Copy link
Member

🚢!

@motinados
Copy link
Contributor Author

motinados commented Oct 19, 2023

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
Would you like me to make the changes?

@severinlandolt
Copy link
Member

Hi @motinados, sorry for the delayed response – I completely missed your comment. Yes, please feel free to make any edits you deem necessary :)

@severinlandolt severinlandolt changed the base branch from main to beta October 24, 2023 22:55
@severinlandolt severinlandolt changed the base branch from beta to main October 24, 2023 22:56
@motinados
Copy link
Contributor Author

@severinlandolt
The changes are done, and the PR is ready for review. Appreciate your time!

@severinlandolt
Copy link
Member

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.

@motinados
Copy link
Contributor Author

I understand the situation, and I'm looking forward to the v4 release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: Breaking Change This PR contains a breaking change PR: In Review This PR is in the process of being reviewed by the team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Icons in next.js 13
2 participants