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

Toolbars: Fall back to name if both title and icon are not specified #17430

Merged
merged 3 commits into from Jul 14, 2022

Conversation

huyenltnguyen
Copy link
Contributor

@huyenltnguyen huyenltnguyen commented Feb 6, 2022

Issue: #15259

What I did

  • Fixed a regression by setting name as the fallback for toolbar title if both icon and title are not specified in globalTypes. I also added a warning to the console to let the users know how to update their code.
  • Added a warning to the console in the case where showName is set to true. This is related to this change where we deprecated the showName config.

How to test

  • Is this testable with Jest or Chromatic screenshots?
  • Does this need a new example in the kitchen sink apps?
  • Does this need an update to the documentation?

Tested with examples/cra-ts-essentials.

Screenshot

When both icon and title are not specified

Before After
Screen Shot 2022-02-06 at 19 12 11 Screen Shot 2022-02-06 at 18 48 34

Browser console:

Screen Shot 2022-02-06 at 18 56 22

When showName is set to true

Browser console:

Screen Shot 2022-02-06 at 18 47 14

@nx-cloud
Copy link

nx-cloud bot commented Feb 6, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 4192168. As they complete they will appear below. Click to see the status, the terminal output, and the build insights.

📂 See all runs for this branch


✅ Successfully ran 1 target

Sent with 💌 from NxCloud.

@huyenltnguyen huyenltnguyen changed the title fix(toolbars): fall back to name if both title and icon are not specified fix(addon-toolbars): fall back to name if both title and icon are not specified Feb 7, 2022
@yannbf yannbf self-assigned this Feb 7, 2022
@ndelangen
Copy link
Member

@huyenltnguyen could you update this PR? I would do it, but I don't have authorization to commit to your fork.

Please merge in the base branch next, so that the CI runs and checks if everything is still compatible.

@yannbf could you review as well?

Copy link
Member

@yannbf yannbf left a comment

Choose a reason for hiding this comment

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

Chromatic borked for a second but I updated the branch and it's all good now. Seems like a great change. Thank you so much for that @huyenltnguyen !

@yannbf yannbf changed the title fix(addon-toolbars): fall back to name if both title and icon are not specified Addon toolbars: fall back to name if both title and icon are not specified Jul 14, 2022
@shilman shilman changed the title Addon toolbars: fall back to name if both title and icon are not specified Toolbars: Fall back to name if both title and icon are not specified Jul 14, 2022
@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Jul 14, 2022
@shilman shilman merged commit 64ede83 into storybookjs:next Jul 14, 2022
@huyenltnguyen huyenltnguyen deleted the fix/toolbars branch July 23, 2022 19:49
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Jul 26, 2022
shilman added a commit that referenced this pull request Jul 26, 2022
Toolbars: Fall back to name if both title and icon are not specified
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: toolbars bug patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants