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

UI: Remove unused & duplicated code #11155

Merged
merged 2 commits into from Jun 14, 2020
Merged

UI: Remove unused & duplicated code #11155

merged 2 commits into from Jun 14, 2020

Conversation

mbelsky
Copy link
Contributor

@mbelsky mbelsky commented Jun 12, 2020

What I did

Hey,

I've found that /lib/ui/src/libs files are unused or have some duplication.

How to test

  • Check lib/ui/src/settings/shortcuts.test.js is passed

@@ -200,7 +203,10 @@ class ShortcutsScreen extends Component<ShortcutsScreenProps, ShortcutsScreenSta

this.setState({
activeFeature: focusedInput,
shortcutKeys: { ...shortcutKeys, [focusedInput]: { shortcut: null, error: false } },
shortcutKeys: {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Auto formatted on pre-commit hook

@@ -3,7 +3,7 @@ import React, { useMemo } from 'react';
import { Badge } from '@storybook/components';
import { API } from '@storybook/api';

import { shortcutToHumanString } from '../libs/shortcut';
import { shortcutToHumanString } from '@storybook/api/dist/lib/shortcut';
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't be importing from dist. can you export these from @storybook/api/shortcut maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be able use the @storybook/api/shortcut import I also need add the shortcut module declaration in lib/api/src/typings.d.ts. And @storybook/api/shortcut won't be typed and won't make auto-suggestions while coding.

As an alternative I can export shortcut from lib/api/src/index.tsx:

import * as shortcut from './lib/shortcut';

export { shortcut };

And use it like

import { shortcut } from '@storybook/api';
const { shortcutToHumanString } = shortcut;

Which way is preferred to make shortcut importable?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@mbelsky mbelsky Jun 13, 2020

Choose a reason for hiding this comment

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

Thank you for this example, fixed

Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Good catch 🤦 ... amazing what sneaks into a large codebase with 1000 contributors. Small comment, otherwise looks great.

@shilman shilman added maintenance User-facing maintenance tasks ui labels Jun 13, 2020
@shilman shilman changed the title Remove unused & duplicated code UI: Remove unused & duplicated code Jun 13, 2020
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

Awesome. Thanks for the fix!!!

@shilman shilman merged commit 1af7ae6 into storybookjs:next Jun 14, 2020
@mbelsky mbelsky deleted the reuse-api-lib branch June 14, 2020 00:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks performance issue ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants