-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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 (core): makeTheme should perform deep merges of objects #1403
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1403 +/- ##
=======================================
Coverage ? 96.89%
=======================================
Files ? 54
Lines ? 4990
Branches ? 825
=======================================
Hits ? 4835
Misses ? 147
Partials ? 8 ☔ View full report in Codecov by Sentry. |
79cb151
to
89f62be
Compare
Hey 👋🏻 Thanks for sending this in! I went with a deeper fix to perform deep merges on the theme objects instead. I think that'll prevent more potential issues like this to occur in the future or in community prompts. I reverted the changes to the checkbox file, but the tests you sent are very useful to make sure we won't introduce future regression. Feel free to review the code (I believe the types in particular could be improved, right now I'm kinda forcing it), I'll merge later! Cheers |
Ah no, I personally don't get this type error. Could you paste the error message? (I'll point out the area in the code) |
} | ||
} | ||
|
||
return output as T; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Those as
uses aren't safe since they force a type. Ideally we could have TS type check without forcing the types.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
I wonder if either:
|
Personally, I'm inclined towards option 2. I will try to figure it out and then report back to you, please hold on. |
When setting the icon for a checkbox:
If only partial values are passed in, it will result in the cursor becoming undefined.
This is primarily because
makeTheme
only performs a deep merge on thestyle
.