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

adamborowski · 2020-05-28T07:48:05Z

What I did

Do not refer to story title in addsMap keys - they are not needed.
Instead use just sanitized story name.
Snapshots were updated and manually compared because cases were reordered due to change in id (title was removed from key)

How to test

Is this testable with Jest or Chromatic screenshots?
Yes, tests are updated
Does this need a new example in the kitchen sink apps?
Yes, one story was added to how dynamic title exapmle
Does this need an update to the documentation?
No this is a bug

If your answer is yes to any of these, please make sure to include it in your PR.

(snapshots were updated and manually compared because cases were reordered due to change in id (title was removed from key)

adamborowski · 2020-05-28T07:49:27Z

examples/official-storybook/stories/addon-docs/dynamic-title.stories.js

@@ -0,0 +1,7 @@
+const getTitle = () => `Addons/Docs/${['dynamic title'][0]}`;


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

adamborowski · 2020-05-28T07:50:31Z

lib/source-loader/src/abstract-syntax-tree/__snapshots__/inject-decorator.test.js.snap

@@ -292,7 +292,28 @@ storiesOf('Custom|ng-content', module).addParameters({ storySource: { source: __
 "
 `;

-exports[`inject-decorator positive - flow calculates "adds" map 1`] = `Object {}`;
+exports[`inject-decorator positive - flow calculates "adds" map 1`] = `


looks like it also fixed existing snapshot that was false positive

adamborowski · 2020-05-28T08:12:05Z

addons/docs/src/blocks/enhanceSource.ts

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


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

adamborowski · 2020-05-28T08:13:00Z

lib/source-loader/src/abstract-syntax-tree/__snapshots__/inject-decorator.test.js.snap

@@ -393,25 +414,7 @@ storiesOf('ngrx|Store', module).addParameters({ storySource: { source: __STORY__

 exports[`inject-decorator positive calculates "adds" map 1`] = `


checked this snapshot manually, we see lot of changes only because items in object were reordered because key was renamed and it orders it alphabetically

adamborowski · 2020-05-30T09:25:23Z

@shilman are you going to review this soon? I want to prepare PR to stable version as well after successful merge of this one. Do you need any more discussion?
Also I see many jobs failing but looking at the logs it is more related to temporary environment issues not the code.

shilman · 2020-05-30T10:39:49Z

Yes I’ll review it in the next week

adamborowski · 2020-06-03T12:28:48Z

@shilman can you just have a quick look for this and say if this is on acceptable level?
If yes, I will prepare PR for v5 as well as we really want to get this fixed in v5 without creating our own fork

shilman

I need to test but looks OK to me on quick inspection. I don’t want to release this in 5.x: it’s painful and time consuming if anything goes wrong in a patch release. 6.0 is closing in on RC so it’s a natural fit there.

ndelangen · 2020-06-04T19:33:01Z

examples/official-storybook/stories/addon-docs/dynamic-title.stories.js

+const getTitle = () => `Addons/Docs/${['dynamic title'][0]}`;
+
+export default {
+  title: getTitle(),


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?

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

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?

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.

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

adamborowski · 2020-06-15T08:57:37Z

Hi. Can you share your plans regarding to this bugfix? It is very important for us

shilman

@adamborowski thanks for your patience on this and for the solid PR.

We don't want to support fully dynamic titles in CSF/MDX. But given that there is not yet any alternative, this is a good fix for the current situation.

The plan:

Merge now & release on beta. RC in ~2 wks.
Explore stylized alternatives fully dynamic titles at some point in the 6.x cycle.
Possibly remove support for fully dynamic titles in 7.0 (be forewarned).

adamborowski · 2020-06-16T05:02:09Z

Thanks for cooperation. I am very interested to see how you imagine maintaining story tree in big monorepos. Do you plan to make some RFC in the future? We can show how we organize story tree using our Babel macro, that may clarify some future requirements.

shilman · 2020-06-16T05:12:33Z

@adamborowski would love to see your requirements. i've created a placeholder issue here #11186 cc @ndelangen @tmeasday

storybookjs#9582 do not refer to story title in addsMap keys

1c8964a

(snapshots were updated and manually compared because cases were reordered due to change in id (title was removed from key)

adamborowski requested a review from usulpro as a code owner May 28, 2020 07:48

adamborowski commented May 28, 2020

View reviewed changes

shilman added addon: docs block: source bug labels May 29, 2020

shilman self-assigned this May 29, 2020

shilman reviewed Jun 3, 2020

View reviewed changes

ndelangen reviewed Jun 4, 2020

View reviewed changes

adamborowski changed the title ~~#9582 support dynamic title for stories~~ #9582 support 'show source' for stories with dynamic titles Jun 5, 2020

adamborowski changed the title ~~#9582 support 'show source' for stories with dynamic titles~~ #9582 support 'show source' for stories with dynamic title Jun 5, 2020

shilman changed the title ~~#9582 support 'show source' for stories with dynamic title~~ Source: Support 'show source' for stories with dynamic title Jun 9, 2020

shilman changed the title ~~Source: Support 'show source' for stories with dynamic title~~ Source: Fix 'show source' for stories with dynamic title Jun 9, 2020

shilman approved these changes Jun 16, 2020

View reviewed changes

shilman added this to the 6.0 docs milestone Jun 16, 2020

shilman changed the title ~~Source: Fix 'show source' for stories with dynamic title~~ Source-loader: Fix 'show source' for stories with dynamic title Jun 16, 2020

shilman changed the title ~~Source-loader: Fix 'show source' for stories with dynamic title~~ Addon-docs: Fix 'show source' for stories with dynamic title Jun 16, 2020

shilman merged commit 912ad04 into storybookjs:next Jun 16, 2020

shilman mentioned this pull request Jun 16, 2020

CSF: Stylized title generation #11186

Closed

shilman mentioned this pull request Jun 23, 2020

Using template strings in a Storybook title does not work with storysource addon #11213

Closed

shilman mentioned this pull request Jul 31, 2020

Docs page doesn't show code preview when default export is a function call #11738

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

adamborowski commented May 28, 2020 •

edited by shilman

Loading

adamborowski May 28, 2020

adamborowski May 28, 2020

adamborowski May 28, 2020

adamborowski May 28, 2020

adamborowski commented May 30, 2020

shilman commented May 30, 2020

adamborowski commented Jun 3, 2020 •

edited

Loading

shilman left a comment

ndelangen Jun 4, 2020

adamborowski Jun 5, 2020 •

edited

Loading

tmeasday Jun 9, 2020

adamborowski Jun 9, 2020 •

edited

Loading

ndelangen Jun 9, 2020

adamborowski commented Jun 15, 2020

shilman left a comment •

edited

Loading

adamborowski commented Jun 16, 2020

shilman commented Jun 16, 2020

		@@ -0,0 +1,7 @@
		const getTitle = () => `Addons/Docs/${['dynamic title'][0]}`;

		@@ -393,25 +414,7 @@ storiesOf('ngrx\|Store', module).addParameters({ storySource: { source: __STORY__

		exports[`inject-decorator positive calculates "adds" map 1`] = `

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

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

Conversation

adamborowski commented May 28, 2020 • edited by shilman Loading

What I did

How to test

adamborowski May 28, 2020

Choose a reason for hiding this comment

adamborowski May 28, 2020

Choose a reason for hiding this comment

adamborowski May 28, 2020

Choose a reason for hiding this comment

adamborowski May 28, 2020

Choose a reason for hiding this comment

adamborowski commented May 30, 2020

shilman commented May 30, 2020

adamborowski commented Jun 3, 2020 • edited Loading

shilman left a comment

Choose a reason for hiding this comment

ndelangen Jun 4, 2020

Choose a reason for hiding this comment

adamborowski Jun 5, 2020 • edited Loading

Choose a reason for hiding this comment

tmeasday Jun 9, 2020

Choose a reason for hiding this comment

adamborowski Jun 9, 2020 • edited Loading

Choose a reason for hiding this comment

ndelangen Jun 9, 2020

Choose a reason for hiding this comment

adamborowski commented Jun 15, 2020

shilman left a comment • edited Loading

Choose a reason for hiding this comment

adamborowski commented Jun 16, 2020

shilman commented Jun 16, 2020

adamborowski commented May 28, 2020 •

edited by shilman

Loading

adamborowski commented Jun 3, 2020 •

edited

Loading

adamborowski Jun 5, 2020 •

edited

Loading

adamborowski Jun 9, 2020 •

edited

Loading

shilman left a comment •

edited

Loading