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

CSF: Deprecate displayName parameter #8775

Merged
merged 13 commits into from
Nov 14, 2019
Merged

CSF: Deprecate displayName parameter #8775

merged 13 commits into from
Nov 14, 2019

Conversation

shilman
Copy link
Member

@shilman shilman commented Nov 9, 2019 •
edited
Loading

Issue: #8756 #8507

What I did

How to test

See updated snapshots

@vercel
Copy link

vercel bot commented Nov 9, 2019 •
edited
Loading

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/jy21cen3w
🌍 Preview: https://monorepo-git-8756-deprecate-display-name.storybook.now.sh

@github-actions
Copy link
Contributor

github-actions bot commented Nov 9, 2019 •
edited
Loading

Fails
🚫 PR is marked with "BREAKING CHANGE" label.

Generated by 🚫 dangerJS against 7b4a6c2

@Hypnosphi Hypnosphi added this to the 6.0.0 milestone Nov 9, 2019
@Hypnosphi
Copy link
Member

Hypnosphi commented Nov 9, 2019 •
edited by shilman
Loading

Thinks to check, based on what I encountered in #8508:

  • Addon Storysource
  • MDX stories

Please also update:

@tmeasday
Copy link
Member

I don't see a deprecation for displayName? Otherwise seems reasonable.

@shilman
Copy link
Member Author

shilman commented Nov 11, 2019 •
edited
Loading

I'd like to release this in 5.3 although it's technically a breaking change.

Here's the rationale:

  • It's a much simpler way to do things
  • The 5.2 CSF change was also a breaking change, and although this subjects existing 5.2 upgraders to two breaking changes (storyshots broke in 5.2 for people who upgraded to CSF, and will break again to fix the problem), I imagine that most people haven't upgraded yet, so the sooner we can get this out the better.

Apologies to semver purists. We're doing our best here! cc @Hypnosphi @igor-dv

@shilman shilman mentioned this pull request Nov 12, 2019
Closed
@shilman shilman modified the milestones: 6.0.0, 5.3.0 Nov 12, 2019
tmeasday
tmeasday requested changes Nov 13, 2019
View reviewed changes
Copy link
Member

@tmeasday tmeasday left a comment

Choose a reason for hiding this comment

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

Coupla small things, LGTM otherwise

MIGRATION.md Outdated Show resolved Hide resolved
lib/core/src/client/preview/start.js Outdated
const displayNameParams = name ? { displayName: name } : {};
kind.add(storyNameFromExport(key), storyFn, {
const exportName = storyNameFromExport(key);
const idParams = { __id: toId(kindName, exportName) };
Copy link
Member

Choose a reason for hiding this comment

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

Should we only pass this if name is defined?

Copy link
Member Author

Choose a reason for hiding this comment

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

Hmm could see arguments both ways. I think it's simpler to do the calculation here so it's all in one place.

lib/client-api/src/client_api.ts Outdated Show resolved Hide resolved
@shilman @tmeasday
Update MIGRATION.md
136845c 
Co-Authored-By: Tom Coleman <[email protected]>
shilman and others added 2 commits November 13, 2019 22:04
@shilman shilman merged commit c2c1b4f into next Nov 14, 2019
@shilman shilman deleted the 8756-deprecate-display-name branch November 14, 2019 07:18
@shilman shilman mentioned this pull request Nov 14, 2019
Closed
@Hypnosphi Hypnosphi mentioned this pull request Jan 1, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmeasday tmeasday tmeasday requested changes

@mrmckeb mrmckeb mrmckeb approved these changes

@alexandrebodin alexandrebodin Awaiting requested review from alexandrebodin

@alterx alterx Awaiting requested review from alterx

@igor-dv igor-dv Awaiting requested review from igor-dv

@thomasbertet thomasbertet Awaiting requested review from thomasbertet

@Hypnosphi Hypnosphi Awaiting requested review from Hypnosphi

@ndelangen ndelangen Awaiting requested review from ndelangen

@aaronmcadam aaronmcadam Awaiting requested review from aaronmcadam

@usulpro usulpro Awaiting requested review from usulpro

Assignees
No one assigned
Labels
BREAKING CHANGE csf
Projects
None yet
Milestone
5.3.0
Development

Successfully merging this pull request may close these issues.
4 participants
@shilman @Hypnosphi @tmeasday @mrmckeb