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

FIX brandImage staying storybook's logo when theme is created with brandTitle #6120

Merged
merged 1 commit into from
Mar 16, 2019

Conversation

ndelangen
Copy link
Member

@ndelangen ndelangen commented Mar 15, 2019

Issue: #5866

What I did

CHANGE theme creation so when setting brandTitle, it sets brandImage to null if not supplied also.

@codecov
Copy link

codecov bot commented Mar 15, 2019

Codecov Report

Merging #6120 into next will decrease coverage by <.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##             next    #6120      +/-   ##
==========================================
- Coverage   37.64%   37.64%   -0.01%     
==========================================
  Files         642      642              
  Lines        9390     9391       +1     
  Branches     1365     1337      -28     
==========================================
  Hits         3535     3535              
  Misses       5295     5295              
- Partials      560      561       +1
Impacted Files Coverage Δ
lib/theming/src/create.ts 87.75% <0%> (-1.83%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 79a5243...9767981. Read the comment docs.

@ndelangen ndelangen requested a review from shilman March 15, 2019 23:37
Copy link
Member

@shilman shilman left a comment

Choose a reason for hiding this comment

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

👍

@shilman shilman merged commit 3f08c31 into next Mar 16, 2019
@shilman shilman deleted the fix/theming-brandImage branch March 16, 2019 00:55
@shilman shilman added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Apr 16, 2019
@shilman shilman added this to the 5.0.x milestone Apr 16, 2019
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label Apr 16, 2019
shilman added a commit that referenced this pull request Apr 16, 2019
FIX brandImage staying storybook's logo when theme is created with brandTitle
@@ -170,7 +170,7 @@ export const convert = (inherit: ThemeVars = lightThemeVars): Theme => {
brand: {
title: brandTitle,
url: brandUrl,
image: brandImage,
image: brandImage || brandTitle ? null : undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't this make brandImage never used? Shouldn't this be

image: brandImage || (brandTitle ? null : undefined),

Copy link
Member

Choose a reason for hiding this comment

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

Good catch 😖

Copy link
Member

Choose a reason for hiding this comment

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

@kennethtruong Mind doing a small PR for this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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 theming
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants