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

Core: Fix api.selectStory for component permalinks #9054

Merged
merged 3 commits into from
Dec 5, 2019
Merged

Core: Fix api.selectStory for component permalinks #9054

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bda3894
Core: Fix `api.selectStory` in the case of component permalinks
shilman Dec 4, 2019
655b419
Update stories.test.js
shilman Dec 4, 2019
a3321ce
Update welcome.stories.js
shilman Dec 4, 2019
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
5 changes: 4 additions & 1 deletion examples/official-storybook/stories/demo/welcome.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,10 @@ export default {
component: Welcome,
};

export const ToStorybook = () => <Welcome showApp={linkTo('Button')} />;
// Some other valid values:
// - 'other-demo-buttonmdx--with-text'
// - 'Other/Demo/ButtonMdx'
export const ToStorybook = () => <Welcome showApp={linkTo('Other/Demo/Button')} />;
shilman marked this conversation as resolved.
Show resolved Hide resolved
ToStorybook.story = {
name: 'to Storybook',
};
14 changes: 13 additions & 1 deletion lib/api/src/modules/stories.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -364,7 +364,19 @@ Did you create a path that uses the separator char accidentally, such as 'Vue <d
const kind = storyId.split('--', 2)[0];
selectStory(toId(kind, story));
} else {
selectStory(toId(kindOrId, story));
const id = toId(kindOrId, story);
if (storiesHash[id]) {
selectStory(id);
} else {
// Support legacy API with component permalinks, where kind is `x/y` but permalink is 'z'
const k = storiesHash[sanitize(kindOrId)];
if (k && k.children) {
const foundId = k.children.find(childId => storiesHash[childId].name === story);
Copy link
Member

Choose a reason for hiding this comment

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

Would it be better to do:

// This kind has a custom id, look up the story using it
if (k && k.id !== sanitize(kindOrId))
  return selectStory(k.id, story)

PS prefer kind to k if the code is more than one line.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Unfortunately k.id is sanitize(kindOrId)
  2. kind was already in scope

Copy link
Member

Choose a reason for hiding this comment

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

Is there nothing stored on the kind in the store to indicate its custom id? Should we change that?

Copy link
Member

Choose a reason for hiding this comment

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

I feel like this line of code is pretty dodgy and we'll regret it in the long run

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that the entire setup is lacking integrity, but I don't have a good idea for a better setup. Any suggestions?

Copy link
Member Author

Choose a reason for hiding this comment

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

Where would this get passed in:

storiesOf({ kind, componentId }, module).add(...);

storiesOf(kind, module).setComponentId(componentId);

???

Copy link
Member

Choose a reason for hiding this comment

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

Oh, so currently we don't store componentId at all, we just change the individual stories ids based on it?

Copy link
Member

Choose a reason for hiding this comment

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

I would suggest setComponentId()

Copy link
Member Author

@shilman shilman Dec 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@tmeasday there's no intermediate data structure in story_store to hold this AFAIK. i feel like this is all getting out of scope.

Copy link
Member

Choose a reason for hiding this comment

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

OK, fair enough.

if (foundId) {
selectStory(foundId);
}
}
}
}
};

Expand Down
78 changes: 75 additions & 3 deletions lib/api/src/tests/stories.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,13 @@ describe('stories API', () => {
path: 'b-d--2',
id: 'b-d--2',
},
'custom-id--1': {
kind: 'b/e',
name: '1',
parameters,
path: 'custom-id--1',
id: 'custom-id--1',
},
};
describe('setStories', () => {
it('stores basic kinds and stories w/ correct keys', () => {
Expand All @@ -74,6 +81,8 @@ describe('stories API', () => {
'b-d',
'b-d--1',
'b-d--2',
'b-e',
'custom-id--1',
]);
expect(storedStoriesHash.a).toMatchObject({
id: 'a',
Expand All @@ -100,7 +109,7 @@ describe('stories API', () => {

expect(storedStoriesHash.b).toMatchObject({
id: 'b',
children: ['b-c', 'b-d'],
children: ['b-c', 'b-d', 'b-e'],
isRoot: false,
isComponent: false,
});
Expand Down Expand Up @@ -144,6 +153,22 @@ describe('stories API', () => {
name: '2',
parameters,
});

expect(storedStoriesHash['b-e']).toMatchObject({
id: 'b-e',
parent: 'b',
children: ['custom-id--1'],
isRoot: false,
isComponent: true,
});

expect(storedStoriesHash['custom-id--1']).toMatchObject({
shilman marked this conversation as resolved.
Show resolved Hide resolved
id: 'custom-id--1',
parent: 'b-e',
kind: 'b/e',
name: '1',
parameters,
});
});

it('sets roots when showRoots = true', () => {
Expand Down Expand Up @@ -496,7 +521,7 @@ describe('stories API', () => {
const {
api: { setStories, jumpToStory },
state,
} = initStories({ store, navigate, storyId: 'b-d--2', viewMode: 'story' });
} = initStories({ store, navigate, storyId: 'custom-id--1', viewMode: 'story' });
store.setState(state);
setStories(storiesHash);

Expand Down Expand Up @@ -570,7 +595,7 @@ describe('stories API', () => {
const {
api: { setStories, jumpToComponent },
state,
} = initStories({ store, navigate, storyId: 'b-d--2', viewMode: 'story' });
} = initStories({ store, navigate, storyId: 'custom-id--1', viewMode: 'story' });
store.setState(state);
setStories(storiesHash);

Expand Down Expand Up @@ -651,5 +676,52 @@ describe('stories API', () => {
selectStory('a');
expect(navigate).toHaveBeenCalledWith('/story/a--1');
});

describe('compnonent permalinks', () => {
it('allows navigating to kind/storyname (legacy api)', () => {
const navigate = jest.fn();
const store = createMockStore();

const {
api: { selectStory, setStories },
state,
} = initStories({ store, navigate });
store.setState(state);
setStories(storiesHash);

selectStory('b/e', '1');
expect(navigate).toHaveBeenCalledWith('/story/custom-id--1');
});

it('allows navigating to component permalink/storyname (legacy api)', () => {
const navigate = jest.fn();
const store = createMockStore();

const {
api: { selectStory, setStories },
state,
} = initStories({ store, navigate });
store.setState(state);
setStories(storiesHash);

selectStory('custom-id', '1');
expect(navigate).toHaveBeenCalledWith('/story/custom-id--1');
});

it('allows navigating to first story in kind on call by kind', () => {
const navigate = jest.fn();
const store = createMockStore();

const {
api: { selectStory, setStories },
state,
} = initStories({ store, navigate });
store.setState(state);
setStories(storiesHash);

selectStory('b/e');
expect(navigate).toHaveBeenCalledWith('/story/custom-id--1');
});
});
});
});