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

Source-loader: Handle template strings in CSF title #8995

Merged
merged 9 commits into from
Dec 2, 2019

Conversation

atanasster
Copy link
Member

@atanasster atanasster commented Nov 28, 2019

Issue: #7932

What I did

Handle case for template string while parsing CSF ast
Added template string example to story

How to test

const stories = 'Stories';
export default {
  title: `Addons/Docs/${stories}`,
};

@atanasster atanasster requested a review from usulpro as a code owner November 28, 2019 11:18
@vercel
Copy link

vercel bot commented Nov 28, 2019

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/storybook/monorepo/hxwf1ve3e
🌍 Preview: https://monorepo-git-fork-atanasster-csf-title-template-string.storybook.now.sh

@shilman shilman changed the title handle template strings in csf title CSF: Handle template strings in CSF title Nov 28, 2019
@shilman shilman changed the title CSF: Handle template strings in CSF title Source-loader: Handle template strings in CSF title Nov 28, 2019
}
// if a tagged template string.
if (titleValue.type === 'TemplateLiteral' && titleValue.quasis.length > 0) {
title = titleValue.quasis[0].value.raw;
Copy link
Member

@ndelangen ndelangen Nov 29, 2019

Choose a reason for hiding this comment

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

I suppose the support is actually quite limited?

import mdxNotes from '../notes/notes.mdx';
import { DocgenButton } from '../../components/DocgenButton';


const stories = () => 'Stories';

export default {
  title: `Addons/Docs/${stories()}`,
  component: DocgenButton,
};

wouldn't actually work?

Copy link
Member Author

@atanasster atanasster Nov 29, 2019

Choose a reason for hiding this comment

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

works, I tried the following more real-life examples:

const docsTitle = title => `Docs/${title}`;
export default {
  title: `Addons/${docsTitle('Stories')}`,
};

or

import { DocgenButton } from '../../components/DocgenButton';
export default {
  title: `Addons/Docs/${DocgenButton.displayName}`,
};

and similar.

For other cases like expressions, dynamic functions etc - I assume there is some library that does ast node value evaluation, but I couldn't find one.

I checked in a better example and made the behavior backwards compatible for non-strings

Copy link
Contributor

Choose a reason for hiding this comment

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

What about paths.macro? Can it evaluate that correctly?

import * as React from 'react'
import base from 'paths.macro'

export default {
  title: `${base}`
}

export const example = () => <div />

Copy link
Member Author

Choose a reason for hiding this comment

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

The evaluation itself works fine. However there are some rules for titles - it can not finish in /.

for example we have:
import base from 'paths.macro'

that evaluates to /stories/addon-docs/

In the case those will work:

title: `Docs${base}Story`,
title: `${base}Story`,

However currently this will not work directly (ends in /):
title: `Docs${base}`

@vercel vercel bot temporarily deployed to staging November 29, 2019 16:03 Inactive
@shilman
Copy link
Member

shilman commented Nov 30, 2019

@atanasster can you check the failing snapshot?

@atanasster
Copy link
Member Author

@shilman - I reverted the snapshot and it works now.

@shilman
Copy link
Member

shilman commented Dec 1, 2019

@atanasster @tmeasday Any idea why these are showing up as new stories in Chromatic? I tried modifying the capitalization to match the previous version, but it didn't seem to change anything.

@atanasster
Copy link
Member Author

@shilman sorry, I don't know chromatic. can I run it locally on my machine and see the errors?

@tmeasday
Copy link
Member

tmeasday commented Dec 1, 2019

@shilman it's because the previous build didn't have the stories with the old capitalisation. Try merging in the latest next build.

@shilman shilman added this to the 5.3.0 milestone Dec 2, 2019
@shilman shilman merged commit 1e53a3a into storybookjs:next Dec 2, 2019
@shilman shilman deleted the csf-title-template-string branch December 2, 2019 03:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants