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

Deprecate confusing option names #1692

Merged
merged 16 commits into from
Aug 31, 2017
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions addons/options/.storybook/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ setOptions({
name: 'CUSTOM-OPTIONS',
url: 'https://github.com/storybooks/storybook',
// goFullScreen: false,
// showLeftPanel: true,
showDownPanel: false,
// showStoriesPanel: true,
showAddonPanel: false,
// showSearchBox: false,
// downPanelInRight: false,
// addonPanelInRight: false,
});

storybook.configure(() => require('./stories'), module);
6 changes: 3 additions & 3 deletions examples/cra-kitchen-sink/.storybook/config.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,10 +6,10 @@ setOptions({
name: 'CRA Kitchen Sink',
url: 'https://github.com/storybooks/storybook/tree/master/examples/cra-kitchen-sink',
goFullScreen: false,
showLeftPanel: true,
showDownPanel: true,
showStoriesPanel: true,
showAddonsPanel: true,
showSearchBox: false,
downPanelInRight: true,
addonsPanelInRight: true,
sortStoriesByKind: false,
hierarchySeparator: /\/|\./,
});
Expand Down
12 changes: 6 additions & 6 deletions lib/ui/src/libs/key_events.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,14 @@ import keycode from 'keycode';

export const features = {
FULLSCREEN: 1,
Copy link
Member

Choose a reason for hiding this comment

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

Maybe it's a good time to also switch from numbers to literals here, what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice idea

DOWN_PANEL: 2,
LEFT_PANEL: 3,
ADDON_PANEL: 2,
NAVIGATION_PANEL: 3,
Copy link
Member

Choose a reason for hiding this comment

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

Why everywhere it's called "Stories Panel" but here is "Navigation Panel"?

Copy link
Member Author

Choose a reason for hiding this comment

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

My bad, skipped when refactoring

SHORTCUTS_HELP: 4,
ESCAPE: 5,
NEXT_STORY: 6,
PREV_STORY: 7,
SHOW_SEARCH: 8,
DOWN_PANEL_IN_RIGHT: 9,
ADDON_PANEL_IN_RIGHT: 9,
};

export function isModifierPressed(e) {
Expand Down Expand Up @@ -42,10 +42,10 @@ export default function handle(e) {
return features.FULLSCREEN;
case keycode('D'):
e.preventDefault();
return features.DOWN_PANEL;
return features.ADDON_PANEL;
case keycode('L'):
e.preventDefault();
return features.LEFT_PANEL;
return features.NAVIGATION_PANEL;
case keycode('right'):
e.preventDefault();
return features.NEXT_STORY;
Expand All @@ -57,7 +57,7 @@ export default function handle(e) {
return features.SHOW_SEARCH;
case keycode('J'):
e.preventDefault();
return features.DOWN_PANEL_IN_RIGHT;
return features.ADDON_PANEL_IN_RIGHT;
default:
return false;
}
Expand Down
2 changes: 1 addition & 1 deletion lib/ui/src/modules/api/actions/api.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -216,7 +216,7 @@ describe('manager.api.actions.api', () => {
expect(stateUpdates.selectedAddonPanel).toEqual('storybooks/storybook-addon-knobs');
});

it('should keep current downPanel and output panel IDs', () => {
it('should keep current addonPanel and output panel IDs', () => {
const clientStore = new MockClientStore();
actions.setOptions({ clientStore, provider }, { selectedAddonPanel: null });

Expand Down
41 changes: 34 additions & 7 deletions lib/ui/src/modules/shortcuts/actions/shortcuts.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,23 +2,32 @@ import pick from 'lodash.pick';
import { features } from '../../../libs/key_events';
import apiActions from '../../api/actions';

const deprecationMessage = (oldName, newName) =>
`The ${oldName} option has been renamed to ${newName} and will not be available in the next major Storybook release. Please update your config.`;

export function keyEventToOptions(currentOptions, event) {
switch (event) {
case features.FULLSCREEN:
return { goFullScreen: !currentOptions.goFullScreen };
case features.DOWN_PANEL:
return { showDownPanel: !currentOptions.showDownPanel };
case features.LEFT_PANEL:
return { showLeftPanel: !currentOptions.showLeftPanel };
case features.ADDON_PANEL:
return { showAddonPanel: !currentOptions.showAddonPanel };
case features.NAVIGATION_PANEL:
return { showStoriesPanel: !currentOptions.showStoriesPanel };
case features.SHOW_SEARCH:
return { showSearchBox: true };
case features.DOWN_PANEL_IN_RIGHT:
return { downPanelInRight: !currentOptions.downPanelInRight };
case features.ADDON_PANEL_IN_RIGHT:
return { addonPanelInRight: !currentOptions.addonPanelInRight };
default:
return {};
}
}

const renamedOptions = {
showLeftPanel: 'showStoriesPanel',
showDownPanel: 'showAddonPanel',
downPanelInRight: 'addonPanelInRight',
};

export default {
handleEvent(context, event) {
const { clientStore } = context;
Expand Down Expand Up @@ -51,8 +60,26 @@ export default {
...pick(options, Object.keys(state.shortcutOptions)),
};

const withNewNames = Object.keys(renamedOptions).reduce((acc, oldName) => {
const newName = renamedOptions[oldName];

if (oldName in options && !(newName in options)) {
if (process.env.NODE_ENV !== 'production') {
// eslint-disable-next-line no-console
console.warn(deprecationMessage(oldName, newName));
}

return {
...acc,
[newName]: options[oldName],
};
}

return acc;
}, updatedOptions);

return {
shortcutOptions: updatedOptions,
shortcutOptions: withNewNames,
};
});
},
Expand Down
29 changes: 29 additions & 0 deletions lib/ui/src/modules/shortcuts/actions/shortcuts.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -35,5 +35,34 @@ describe('manager.shortcuts.actions.shortcuts', () => {
shortcutOptions: { bbc: 50, abc: 10 },
});
});

test('should warn about deprecated option names', () => {
const clientStore = new MockClientStore();
const spy = jest.spyOn(console, 'warn');
actions.setOptions(
{ clientStore },
{
showLeftPanel: 1,
showDownPanel: 2,
downPanelInRight: 3,
}
);

const state = {
shortcutOptions: {},
};
const stateUpdates = clientStore.updateCallback(state);
expect(spy).toHaveBeenCalledTimes(3);
expect(stateUpdates).toEqual({
shortcutOptions: {
showStoriesPanel: 1,
showAddonPanel: 2,
addonPanelInRight: 3,
},
});

spy.mockReset();
spy.mockRestore();
});
});
});
6 changes: 3 additions & 3 deletions lib/ui/src/modules/shortcuts/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ export default {
defaultState: {
shortcutOptions: {
goFullScreen: false,
showLeftPanel: true,
showDownPanel: true,
showStoriesPanel: true,
showAddonPanel: true,
showSearchBox: false,
downPanelInRight: false,
addonPanelInRight: false,
},
},
};
2 changes: 1 addition & 1 deletion lib/ui/src/modules/ui/actions/ui.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export default {
clientStore.toggle('showShortcutsHelp');
},

selectDownPanel({ clientStore }, panelName) {
selectAddonPanel({ clientStore }, panelName) {
clientStore.set('selectedAddonPanel', panelName);
},
};
4 changes: 2 additions & 2 deletions lib/ui/src/modules/ui/actions/ui.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,13 +24,13 @@ describe('manager.ui.actions.ui', () => {
});
});

describe('selectDownPanel', () => {
describe('selectAddonPanel', () => {
it('should set the given panel name', () => {
const clientStore = {
set: jest.fn(),
};
const panelName = 'kkkind';
actions.selectDownPanel({ clientStore }, panelName);
actions.selectAddonPanel({ clientStore }, panelName);

expect(clientStore.set).toHaveBeenCalledWith('selectedAddonPanel', panelName);
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import PropTypes from 'prop-types';
import React, { Component } from 'react';
import style from './style';

class DownPanel extends Component {
class AddonPanel extends Component {
renderTab(name, panel) {
let tabStyle = style.tablink;
if (this.props.selectedPanel === name) {
Expand Down Expand Up @@ -69,16 +69,16 @@ class DownPanel extends Component {
}
}

DownPanel.defaultProps = {
AddonPanel.defaultProps = {
panels: {},
onPanelSelect: () => {},
selectedPanel: null,
};

DownPanel.propTypes = {
AddonPanel.propTypes = {
panels: PropTypes.object, // eslint-disable-line react/forbid-prop-types
onPanelSelect: PropTypes.func,
selectedPanel: PropTypes.string,
};

export default DownPanel;
export default AddonPanel;
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
import React from 'react';
import { shallow } from 'enzyme';
import DownPanel from './index';
import AddonPanel from './index';

describe('manager.ui.components.down_panel.index', () => {
describe('manager.ui.components.addon_panel.index', () => {
test('should render only the selected panel with display set other than "none"', () => {
const panels = {
test1: {
Expand All @@ -20,7 +20,7 @@ describe('manager.ui.components.down_panel.index', () => {
const onPanelSelect = () => 'onPanelSelect';

const wrapper = shallow(
<DownPanel panels={panels} onPanelSelect={onPanelSelect} selectedPanel={'test2'} />
<AddonPanel panels={panels} onPanelSelect={onPanelSelect} selectedPanel={'test2'} />
);

expect(wrapper.find('#test1').parent()).toHaveStyle('display', 'none');
Expand All @@ -38,7 +38,7 @@ describe('manager.ui.components.down_panel.index', () => {
const onPanelSelect = jest.fn();
const preventDefault = jest.fn();
const wrapper = shallow(
<DownPanel panels={panels} onPanelSelect={onPanelSelect} selectedPanel={'test1'} />
<AddonPanel panels={panels} onPanelSelect={onPanelSelect} selectedPanel={'test1'} />
);
wrapper.find('a').simulate('click', { preventDefault });

Expand All @@ -50,7 +50,7 @@ describe('manager.ui.components.down_panel.index', () => {
test('should render "no panels available"', () => {
const panels = {};
const onPanelSelect = () => 'onPanelSelect';
const wrapper = shallow(<DownPanel panels={panels} onPanelSelect={onPanelSelect} />);
const wrapper = shallow(<AddonPanel panels={panels} onPanelSelect={onPanelSelect} />);

expect(wrapper.contains('no panels available')).toBe(true);
});
Expand Down
Loading