-
Notifications
You must be signed in to change notification settings - Fork 30.1k
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 @theme-ui/color types #42425
Fix @theme-ui/color types #42425
Conversation
@allanpope Thank you for submitting this PR! Because this PR doesn't have any code reviewers, a DefinitelyTyped maintainer will be reviewing it in the next few days once the Travis CI build passes. In the meantime, if the build fails or a merge conflict occurs, I'll let you know. Have a nice day! |
TBH, all of these functions are 2nd order and take theme parameter at the second invocation. I'm also not sure if the tests are right https://theme-ui.com/packages/color/. Instead of Take a look at |
@hasparus Yeah, so it takes a theme color eg: 'primary' as the first parameter. Within the function it does a Are you suggesting that I pull out the theme color names from the theme object and then use them? eg:
I didn't know that you had opened a PR on theme-ui for this. Since theme-ui will have built in types, I may as well close this PR, use yours once it's merged in and remove this package from DT. |
@allanpope The Travis CI build failed! Please review the logs for more information. Once you've pushed the fixes, the build will automatically re-run. Thanks! |
I'll close this for now as mentioned in the comments. If you wanted to have this merged, feel free to reopen or send another PR. |
@hasparus Thanks for picking up that the parameter order was wrong, totally missed that 🤦♂.
Have fixed up the parameter order, added more tests and marked number in the
mix
function as optional, as the source has a default param.Please fill in this template.
npm test
.)npm run lint package-name
(ortsc
if notslint.json
is present).Select one of these and delete the others:
If changing an existing definition: