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.

Sign up for GitHub

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

Jump to bottom

Migrate addon links to TS #6246

Merged
merged 6 commits into from
Mar 25, 2019
Merged

Migrate addon links to TS #6246

merged 6 commits into from
Mar 25, 2019

Conversation

sairus2k
Copy link
Member

Issue: #5030

What I did

Migrate @storybook/addon-links to TypeScript

@sairus2k sairus2k requested a review from ndelangen as a code owner March 23, 2019 14:00
@Armanio Armanio added maintenance User-facing maintenance tasks addon: links typescript labels Mar 23, 2019
@kroeder kroeder mentioned this pull request Mar 23, 2019
Closed
@kroeder
Copy link
Member

kroeder commented Mar 23, 2019

Hey, thanks a lot for starting another TS migration! Do you need any help? 🙂

@ndelangen
Copy link
Member

Seems there are some issues here:

ERR! => Failed to build the preview
ERR! /tmp/storybook/examples/angular-cli/src/stories/addon-links.stories.ts
ERR! ERROR in /tmp/storybook/examples/angular-cli/src/stories/addon-links.stories.ts(9,14):
ERR! TS2554: Expected 2 arguments, but got 1.
ERR! /tmp/storybook/examples/angular-cli/src/stories/index.ts

@ndelangen ndelangen self-assigned this Mar 25, 2019
@codecov
Copy link

codecov bot commented Mar 25, 2019

Codecov Report

Merging #6246 into next will increase coverage by 0.02%.
The diff coverage is 76.19%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6246      +/-   ##
==========================================
+ Coverage   38.16%   38.19%   +0.02%     
==========================================
  Files         645      646       +1     
  Lines        9655     9672      +17     
  Branches      355      365      +10     
==========================================
+ Hits         3685     3694       +9     
- Misses       5915     5920       +5     
- Partials       55       58       +3
Impacted Files Coverage Δ
addons/links/src/constants.ts 0% <ø> (ø)
addons/links/src/manager.ts 0% <ø> (ø)
addons/links/src/react/index.ts 0% <0%> (ø)
addons/links/src/index.ts 0% <0%> (ø)
addons/links/src/react/components/link.tsx 86.66% <87.5%> (ø)
addons/links/src/preview.ts 63.88% <90%> (ø)

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 bbec9a0...1cc8945. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 25, 2019 •
edited
Loading

Codecov Report

Merging #6246 into next will increase coverage by 0.02%.
The diff coverage is 77.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6246      +/-   ##
==========================================
+ Coverage   38.09%   38.12%   +0.02%     
==========================================
  Files         645      646       +1     
  Lines        9681     9698      +17     
  Branches      356      366      +10     
==========================================
+ Hits         3688     3697       +9     
- Misses       5939     5944       +5     
- Partials       54       57       +3
Impacted Files Coverage Δ
addons/links/src/constants.ts 0% <ø> (ø)
addons/links/src/manager.ts 0% <ø> (ø)
addons/links/src/react/index.ts 0% <0%> (ø)
addons/links/src/index.ts 0% <0%> (ø)
...s/storyshots-core/src/api/integrityTestTemplate.js 100% <100%> (ø) ⬆️
addons/links/src/react/components/link.tsx 86.66% <87.5%> (ø)
addons/links/src/preview.ts 63.88% <90%> (ø)

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 301d578...17fa318. Read the comment docs.

@ndelangen ndelangen merged commit 6a1bebc into storybookjs:next Mar 25, 2019
@sairus2k sairus2k deleted the ts-migration/addon-links branch March 25, 2019 13:44
Hypnosphi
Hypnosphi reviewed May 19, 2019
View reviewed changes
addons/links/src/preview.ts
const linksListener = e => {
const { sbKind: kind, sbStory: story } = e.target.dataset;
const linksListener = (e: SyntheticEvent<HTMLLinkElement>) => {
const { sbKind: kind, sbStory: story } = e.currentTarget.dataset;
Copy link
Member

Choose a reason for hiding this comment

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

currentTarget is always document which makes it throw on each click, see e.g.

https://next--storybooks-html.netlify.com/?path=/story/welcome--welcome

@Hypnosphi Hypnosphi mentioned this pull request May 19, 2019
Merged
@ndelangen ndelangen added this to the 5.2.0 milestone May 19, 2019
Hypnosphi
Hypnosphi reviewed May 19, 2019
View reviewed changes
addons/links/src/react/components/link.tsx
this.handleClick = this.handleClick.bind(this);
}
export default class LinkTo extends PureComponent<Props, State> {
defaultProps: Props = {
Copy link
Member

Choose a reason for hiding this comment

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

forgot static?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hypnosphi Hypnosphi Hypnosphi left review comments

@ndelangen ndelangen ndelangen approved these changes

@igor-dv igor-dv Awaiting requested review from igor-dv

@thomasbertet thomasbertet Awaiting requested review from thomasbertet

Assignees

@ndelangen ndelangen

Labels
addon: links maintenance User-facing maintenance tasks typescript
Projects
None yet
Milestone
5.2.0
Development

Successfully merging this pull request may close these issues.
5 participants
@sairus2k @kroeder @ndelangen @Hypnosphi @Armanio