-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Migrate @storybook/addon-jest to TS #6403
Conversation
This pull request is automatically deployed with Now. Latest deployment for this branch: https://monorepo-git-fork-sairus2k-ts-migration-addon-jest.storybook.now.sh |
Codecov Report
@@ Coverage Diff @@
## next #6403 +/- ##
==========================================
+ Coverage 41.04% 41.11% +0.06%
==========================================
Files 613 611 -2
Lines 8454 8440 -14
Branches 378 410 +32
==========================================
Hits 3470 3470
+ Misses 4929 4915 -14
Partials 55 55
Continue to review full report at Codecov.
|
import PropTypes from 'prop-types'; | ||
import React from 'react'; | ||
import { styled } from '@storybook/theming'; | ||
import colors from '../colors'; |
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.
I think these colours should come from the theme if possible
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.
Hmm, I don't find similar colors in lib/theming/src/base.ts
addons/jest/src/styles.js → addons/jest/src/styles.ts I'm unsure if this is still used, I can't find anything tht would use this. I think we should delete this, or at the very least add a deprecation so we can remove it in a next release |
This is great! thanks for taking this on @sairus2k ! I think it works well and is merge-able. We could do a bit more cleanup of this addon, how do you feel about doing that in this PR? |
Looks like you also need to remove
|
# Conflicts: # addons/jest/package.json
# Conflicts: # addons/jest/src/styles.js
{msg | ||
.filter(item => typeof item !== 'string' || item.trim() !== '') | ||
.map((item, index, list) => { | ||
if (typeof item === 'string') { |
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.
seems odd you filter for anything that isn't a string, then do something for only strings. seems like this code could be clearer
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.
It filters for not empty strings, then removes line breaks for those strings
.map((i, index) => (index % 2 ? <Negative key={`n_${li}_${i}`}>{i}</Negative> : i)) | ||
: item | ||
) | ||
.reduce<MsgElement[]>((acc, item) => acc.concat(item), []) |
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.
.reduce<MsgElement[]>((acc, item) => acc.concat(item), []) | |
.reduce((acc, item) => acc.concat(item), [] as MsgElement[]) |
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.
In this case TS throws error Property 'concat' does not exist on type Element
# Conflicts: # addons/jest/package.json
Issue: #5030
What I did
createSubgroup
function to more types-friendlyMessage
component into separate file