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

Migrate addon backgrounds to TS #5535

Merged
merged 5 commits into from Feb 11, 2019
Merged

Migrate addon backgrounds to TS #5535

merged 5 commits into from Feb 11, 2019

Conversation

gaetanmaisse
Copy link
Member

@gaetanmaisse gaetanmaisse commented Feb 10, 2019

Issue:

#5030

What I did

  • Migrate @storybook/addon-backgrounds to TypeScript
  • Refactor some imports/exports
  • Add some types and interfaces and use them to type codebase
  • Rename src/Tool.js to containers/BackgroundSelector.ts
  • Export ColorIcon in its own file components/ColorIcon.ts

@gaetanmaisse
Copy link
Member Author

⚠️ Build will fail, due to TS compilation errors, until #5534 will be merged

Copy link
Contributor

@Keraito Keraito left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

title: string;
onClick: () => void;
value: string;
right?: any;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your changes at https://github.com/storybooks/storybook/pull/5535/files#diff-f01e7bd996316ee937af51de4ab40f37R30 are not quite the same as the original. The original would be right? any, while yours is actually right: any since you're always setting the right property.

Also: Do we not have config in place to type this as a React/JSX element?

@ndelangen ndelangen self-assigned this Feb 11, 2019
@codecov
Copy link

codecov bot commented Feb 11, 2019

Codecov Report

Merging #5535 into next will decrease coverage by 0.05%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##            next    #5535      +/-   ##
=========================================
- Coverage   33.1%   33.05%   -0.06%     
=========================================
  Files        638      640       +2     
  Lines       9264     9279      +15     
  Branches    1318     1346      +28     
=========================================
  Hits        3067     3067              
- Misses      5571     5597      +26     
+ Partials     626      615      -11
Impacted Files Coverage Δ
addons/backgrounds/src/deprecated.ts 0% <ø> (ø)
addons/backgrounds/src/containers/index.ts 0% <0%> (ø)
addons/backgrounds/src/index.ts 0% <0%> (ø)
addons/backgrounds/src/register.tsx 0% <0%> (ø)
addons/backgrounds/src/constants.ts 0% <0%> (ø)
.../backgrounds/src/containers/BackgroundSelector.tsx 0% <0%> (ø)
addons/backgrounds/src/components/ColorIcon.tsx 0% <0%> (ø)
addons/backgrounds/src/components/index.ts 0% <0%> (ø)
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef090d4...9cc4b64. Read the comment docs.

@ndelangen ndelangen added this to the v5.0.0 milestone Feb 11, 2019
@ndelangen ndelangen merged commit 06e3e8d into storybookjs:next Feb 11, 2019
@ndelangen
Copy link
Member

@gaetanmaisse I invited you to the Storybook organisation on GitHub, Would you be able to join our discord to chat?

@gaetanmaisse gaetanmaisse deleted the ts-migration/addon-backgrounds branch February 12, 2019 06:01
@shilman shilman removed this from the v5.0.0 milestone Feb 15, 2019
@shilman
Copy link
Member

shilman commented Feb 22, 2019

@ndelangen @gaetanmaisse @tmeasday FYI I'm going to release this in 5.0 since it's blocking a couple PRs. I'd rather not release it because it was merged after "feature freeze" but i think it's faster to merge than to stick to principle on this one.

@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Feb 23, 2019
shilman pushed a commit that referenced this pull request Feb 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: backgrounds maintenance User-facing maintenance tasks patch:done Patch/release PRs already cherry-picked to main/release branch typescript
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants