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

Addon-docs: Fix 'show source' for stories with dynamic title #10959

Merged
merged 1 commit into from
Jun 16, 2020
Merged

Addon-docs: Fix 'show source' for stories with dynamic title #10959

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion addons/docs/src/blocks/enhanceSource.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ describe('addon-docs enhanceSource', () => {
storySource: {
source: 'storySource.source',
locationsMap: {
'foo--bar': { startBody: { line: 1, col: 5 }, endBody: { line: 1, col: 11 } },
bar: { startBody: { line: 1, col: 5 }, endBody: { line: 1, col: 11 } },
},
},
},
Expand Down
12 changes: 9 additions & 3 deletions addons/docs/src/blocks/enhanceSource.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,20 @@ interface StorySource {
locationsMap: { [id: string]: { startBody: Location; endBody: Location } };
}

/**
* Replaces full story id name like: story-kind--story-name -> story-name
* @param id
*/
const storyIdToSanitizedStoryName = (id: string) => id.replace(/^.*?--/, '');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't like this function but we can't use context.name as it can be overriden by CSF. The id is the only place that contains original sanitized name but it also contains story title (before -- double dash).
So I simply remove sanitized story title from id and get sanitized name


const extract = (targetId: string, { source, locationsMap }: StorySource) => {
if (!locationsMap) {
return source;
}
const location = locationsMap[targetId];

// FIXME: bad locationsMap generated for module export functions whose titles are overridden
if (!location) return null;
const sanitizedStoryName = storyIdToSanitizedStoryName(targetId);
const location = locationsMap[sanitizedStoryName];

const { startBody: start, endBody: end } = location;
const lines = source.split('\n');
if (start.line === end.line && lines[start.line - 1] !== undefined) {
Expand Down
7 changes: 7 additions & 0 deletions examples/official-storybook/stories/addon-docs/dynamic-title.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
const getTitle = () => `Addons/Docs/${['dynamic title'][0]}`;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can use something more 'dynamic' but I think this is enough


export default {
title: getTitle(),
Copy link
Member

Choose a reason for hiding this comment

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

I personally think this is a bad idea, sure it's expressive, but we're trying to make CSF simple and statically analyzable as much as possible.

@shilman @tmeasday what are your opinion on supporting this?

Does supporting this add complexity?
Is it worth it?
To what degree are dynamic titles ok?

Copy link
Contributor Author

@adamborowski adamborowski Jun 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

I think that dynamic story titles are very common. Especially in big monorepo projects. The proposed PR doesn't directly support this it only fills the gap. I didn't see any docs in storybook about limitations to title, but indeed I could see a section in docs about using babel macros to generate title. So now it looks like feature gap.

Please look at this:

https://storybook.js.org/docs/basics/writing-stories/#generating-nesting-path-based-on-__dirname

#9582

Copy link
Member

Choose a reason for hiding this comment

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

I think the last discussion we had about it we were tending to lean towards rather than allowing fully dynamic titles to instead to look at common systems of generating them. The primary one is of course setting titles based on filesystem paths -- are there other ways people are setting dynamic titles apart from purely based on the path of the file?

Copy link
Contributor Author

@adamborowski adamborowski Jun 9, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

We also include package.json name.
But is this PR relevant place to discuss it?

I understand that you discuss about the approach to generate titles but I don't see any other ways to fix the source code button rather than not relying on final id because looks like it is not necessary. Even if you decide to limit somehow the freedom of generating titles.

Copy link
Member

Choose a reason for hiding this comment

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

I'm guessing we should merge this and discuss options on what to do with the above

};

export const basic = () => 'Story with title that is evaluated in runtime';
2 changes: 1 addition & 1 deletion lib/source-loader/src/abstract-syntax-tree/__snapshots__/inject-decorator.csf.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ exports[`inject-decorator positive - ts - csf includes storySource parameter in
import { action } from \\"@storybook/addon-actions\\";
import { Button } from \\"@storybook/react/demo\\";

export default {parameters: {\\"storySource\\":{\\"source\\":\\"import React from \\\\\\"react\\\\\\";\\\\nimport { action } from \\\\\\"@storybook/addon-actions\\\\\\";\\\\nimport { Button } from \\\\\\"@storybook/react/demo\\\\\\";\\\\n\\\\nexport default {\\\\n title: \\\\\\"Button\\\\\\",\\\\n excludeStories: [\\\\\\"text\\\\\\"],\\\\n includeStories: /emoji.*/\\\\n};\\\\n\\\\nexport const text = () => (\\\\n <Button onClick={action(\\\\\\"clicked\\\\\\")}>Hello Button</Button>\\\\n);\\\\n\\\\nexport const emoji = () => (\\\\n <Button onClick={action(\\\\\\"clicked\\\\\\")}>\\\\n <span role=\\\\\\"img\\\\\\" aria-label=\\\\\\"so cool\\\\\\">\\\\n 😀 😎 👍 💯\\\\n </span>\\\\n </Button>\\\\n);\\\\n\\\\nexport function emojiFn() {\\\\n return (\\\\n <Button onClick={action(\\\\\\"clicked\\\\\\")}>\\\\n <span role=\\\\\\"img\\\\\\" aria-label=\\\\\\"so cool\\\\\\">\\\\n 😀 😎 👍 💯\\\\n </span>\\\\n </Button>\\\\n )\\\\n};\\\\n\\",\\"locationsMap\\":{\\"button--text\\":{\\"startLoc\\":{\\"col\\":20,\\"line\\":11},\\"endLoc\\":{\\"col\\":1,\\"line\\":13},\\"startBody\\":{\\"col\\":20,\\"line\\":11},\\"endBody\\":{\\"col\\":1,\\"line\\":13}},\\"button--emoji\\":{\\"startLoc\\":{\\"col\\":21,\\"line\\":15},\\"endLoc\\":{\\"col\\":1,\\"line\\":21},\\"startBody\\":{\\"col\\":21,\\"line\\":15},\\"endBody\\":{\\"col\\":1,\\"line\\":21}},\\"button--emoji-fn\\":{\\"startLoc\\":{\\"col\\":7,\\"line\\":23},\\"endLoc\\":{\\"col\\":1,\\"line\\":31},\\"startBody\\":{\\"col\\":7,\\"line\\":23},\\"endBody\\":{\\"col\\":1,\\"line\\":31}}}},},
export default {parameters: {\\"storySource\\":{\\"source\\":\\"import React from \\\\\\"react\\\\\\";\\\\nimport { action } from \\\\\\"@storybook/addon-actions\\\\\\";\\\\nimport { Button } from \\\\\\"@storybook/react/demo\\\\\\";\\\\n\\\\nexport default {\\\\n title: \\\\\\"Button\\\\\\",\\\\n excludeStories: [\\\\\\"text\\\\\\"],\\\\n includeStories: /emoji.*/\\\\n};\\\\n\\\\nexport const text = () => (\\\\n <Button onClick={action(\\\\\\"clicked\\\\\\")}>Hello Button</Button>\\\\n);\\\\n\\\\nexport const emoji = () => (\\\\n <Button onClick={action(\\\\\\"clicked\\\\\\")}>\\\\n <span role=\\\\\\"img\\\\\\" aria-label=\\\\\\"so cool\\\\\\">\\\\n 😀 😎 👍 💯\\\\n </span>\\\\n </Button>\\\\n);\\\\n\\\\nexport function emojiFn() {\\\\n return (\\\\n <Button onClick={action(\\\\\\"clicked\\\\\\")}>\\\\n <span role=\\\\\\"img\\\\\\" aria-label=\\\\\\"so cool\\\\\\">\\\\n 😀 😎 👍 💯\\\\n </span>\\\\n </Button>\\\\n )\\\\n};\\\\n\\",\\"locationsMap\\":{\\"text\\":{\\"startLoc\\":{\\"col\\":20,\\"line\\":11},\\"endLoc\\":{\\"col\\":1,\\"line\\":13},\\"startBody\\":{\\"col\\":20,\\"line\\":11},\\"endBody\\":{\\"col\\":1,\\"line\\":13}},\\"emoji\\":{\\"startLoc\\":{\\"col\\":21,\\"line\\":15},\\"endLoc\\":{\\"col\\":1,\\"line\\":21},\\"startBody\\":{\\"col\\":21,\\"line\\":15},\\"endBody\\":{\\"col\\":1,\\"line\\":21}},\\"emoji-fn\\":{\\"startLoc\\":{\\"col\\":7,\\"line\\":23},\\"endLoc\\":{\\"col\\":1,\\"line\\":31},\\"startBody\\":{\\"col\\":7,\\"line\\":23},\\"endBody\\":{\\"col\\":1,\\"line\\":31}}}},},
title: \\"Button\\",
excludeStories: [\\"text\\"],
includeStories: /emoji.*/
Expand Down
Loading