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: static and dynamic children #792

Merged

Conversation

motinados
Copy link
Contributor

Description
The cause of this error is the constraint on the children's type. To resolve it, the type of children has been changed to ReactNode.

Related issue(s)
Fixes #686

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

If yes, please describe the impact and migration path for existing applications:

How has This been tested?
jest and storybook

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.

Copy link

vercel bot commented Nov 6, 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 Nov 7, 2023 8:48pm

@motinados motinados changed the title Fix/static and dynamic children fix: static and dynamic children Nov 6, 2023
@severinlandolt severinlandolt changed the base branch from main to beta November 7, 2023 20:36
@severinlandolt severinlandolt added the PR: In Review This PR is in the process of being reviewed by the team label Nov 7, 2023
@severinlandolt
Copy link
Member

Man, you are as fast as it gets! Was about to fix the tests and got notified:
CleanShot 2023-11-07 at 21 49 52

@motinados motinados marked this pull request as draft November 7, 2023 21:11
@motinados
Copy link
Contributor Author

I will return it to draft for now as there is something of concern.

@motinados motinados marked this pull request as ready for review November 8, 2023 20:36
@motinados
Copy link
Contributor Author

@severinlandolt @mbauchet
It seemed like it wasn't working on my local Next.js app, but it turned out to be a misunderstanding on my part. Could you please review the pull request?
Thanks,

@severinlandolt
Copy link
Member

severinlandolt commented Nov 9, 2023

Thank you very much, your contributions are highly appreciated @motinados! 🙏

@severinlandolt severinlandolt merged commit d2f1604 into tremorlabs:beta Nov 9, 2023
5 checks passed
Copy link

github-actions bot commented Nov 9, 2023

🎉 This PR is included in version 3.11.1-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@severinlandolt
Copy link
Member

severinlandolt commented Nov 10, 2023

Hi @motinados, the mapping is now working fine locally and in my test projects.

There has been one issue with unresolved promises. E.g. if data comes from Tanstack query it will be undefined until the promise is resolved. Resulting in this error:

TypeError: Cannot read properties of null (reading 'props')
at selectutils
at mapIntoArray
at mapIntoArray
at Object.mapChildren [as map]
at 011 selectutils
at updateMemo 
at Object.useMemo
at useMemo

CleanShot 2023-11-10 at 17 13 34

mbauchet pointed out, that the problem might be the constructValueToNameMapping function.

Could you take a look at this issue? Feel free to create a new branch from beta

@motinados
Copy link
Contributor Author

@severinlandolt @mbauchet
Thank you for your feedback. However, I am unable to reproduce the issue here.

I did encounter a similar error when the data retrieved with Tanstack query contains null, but it appears at the value={String(coworker.fullname)} in SearchSelectItem.

Since undefined until the promise is resolved filtered out using isValidElement, I believe it should not be a problem.

Could there be some cache remaining? If you are using a Next.js app, please try deleting the .next folder and testing again.

severinlandolt added a commit that referenced this pull request Nov 13, 2023
)

* fix: select height and autofocus (#796)

* add autofocus handler

* Fix selection height

* fix: static and dynamic children (#792)

* fix: Switch color (#802)

---------

Co-authored-by: motinados <69296148+motinados@users.noreply.github.com>
Co-authored-by: mbauchet <90607026+mbauchet@users.noreply.github.com>
Copy link

github-actions bot commented Dec 2, 2023

🎉 This PR is included in version 3.12.0-beta.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

github-actions bot commented Dec 2, 2023

🎉 This PR is included in version 3.12.0-beta-package-updates.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

Copy link

🎉 This PR is included in version 3.12.0-beta-customcolors.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@motinados motinados deleted the fix/static-and-dynamic-children branch December 10, 2023 19:59
@severinlandolt
Copy link
Member

@motinados Heya! One user reported an issue about the children prop. I don't quite understand what is going wrong, because lit looks OK to me actually:

#859

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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]: weird typescript error when mapping SearchSelectItems in SearchSelect component
3 participants