-
Notifications
You must be signed in to change notification settings - Fork 205
feat: add palette file, add palette tests #566
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
base: main
Are you sure you want to change the base?
Conversation
|
|
argyleink
left a comment
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.
❤️ thanks for this!
you've got the files in there, and the tests look like they'll pass, but the one forgotten part was to add them to the main bundle and main JS object.
|
|
||
| test('Should have an all included import', t => { | ||
| t.is(Object.keys(OpenProps).length, 1778) | ||
| t.is(Object.keys(OpenProps).length, 1816) |
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.
main bundle doesnt include the new props because they're not in here https://github.com/argyleink/open-props/blob/main/build/props.js#L34
| t.assert(OpenProps.orange0) | ||
| }) | ||
|
|
||
| test('Import should have palette', async 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.
same with this test, it'll pass once the palette props are part of the JS main bundle
I'm not sure if the tests are used anymore, they are missing some stuff so I could remove that part of the PR (or fix them) - I didn't put much work into it before checking with you.
Step 2 in this PR is for sure the documentation, or it could be a completely new PR.