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

ndelangen commented Jul 2, 2022

@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?

yannbf
yannbf approved these changes Jul 13, 2022
Copy link
Member

@yannbf yannbf left a comment

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 Bugfix & documentation PR that need to be picked to master branch label Jul 14, 2022
@shilman shilman merged commit 64ede83 into storybookjs:next Jul 14, 2022
40 of 42 checks passed
@huyenltnguyen huyenltnguyen deleted the fix/toolbars branch Jul 23, 2022
@shilman shilman added the picked Patch/release PRs cherry-picked to master/release branch label Jul 26, 2022
shilman added a commit that referenced this issue 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 Bugfix & documentation PR that need to be picked to master branch picked Patch/release PRs cherry-picked to master/release branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants