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: addons-background not updating default value #18456

Closed
wants to merge 1 commit into from

Conversation

bytrangle
Copy link
Contributor

Issue: fixes #15632

What I did

  • I checked if the globals object has the backgrounds property
  • If yes, read the default background color setting from backgroundsConfig
  • Populate the globals object with default background color

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?

If your answer is yes to any of these, please make sure to include it in your PR.

@nx-cloud
Copy link

nx-cloud bot commented Jun 10, 2022

☁️ Nx Cloud Report

CI is running/has finished running commands for commit 8bbedba. 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.

@MichaelArestad MichaelArestad added maintenance User-facing maintenance tasks and removed linear labels Jun 13, 2022
@ndelangen
Copy link
Member

@bytrangle So here's what I think should happen:

global background should have no value (null).
parameters can contain a background, if the parameter contains a background we should check if globals has a value.
If it does not have a value we can set the background.

If globals does have a background, a user has manually set this background, and storybook should respect this background.

In other words: globals trump parameters.

What I can see what's incorrect today is when the user sets a background via the ui (sets globals) then clears the background, the value in globals should become null again, but instead it becomes 'transparent'.

Comment on lines +85 to +87
globals[BACKGROUNDS_PARAM_KEY] = {
value: defaultSetting.value,
};
Copy link
Member

Choose a reason for hiding this comment

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

This isn't right, we should not be mutating globals

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is calling updateGlobals a good solution?

Copy link
Member

Choose a reason for hiding this comment

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

No we should not put the background value coming from parameters into the global.

What we want to do instead is read both in the place where we apply the background:

If the global.background is set, apply the CSS value to that.
if the global.background is null, but the parameter is set, apply the CSS value to that.
if neither have a valid value, clear the CSS styles applied, so there is no background color applied at all.

@bytrangle
Copy link
Contributor Author

bytrangle commented Jun 14, 2022

@ndelangen Upon further investigation, there are two important values that BackgroundSelector tracks on initial rendering, before a user sets the background color via UI:

  • globalsBackgroundColor is undefined throughout because user didn't declare a globalType
  • selectedBackgroundColor is initially "transparent", then it is set to the correct default color from parameters.

So the fact that global background color becomes transparent again when a user clears the background via UI seems like the expected behaviour IMO.

But the original issue that my PR spawns from is about a decorator getting empty object when calling context.globals. Is this an issue?

@bytrangle
Copy link
Contributor Author

@ndelangen On further consideration, I think there are two potential complications with setting global background as null:

  1. The type for global background will be either null or string. Is this desirable?
  2. In CSS, there's no such thing as background-color: none or background-color: null. If you want the background to have no color, you set background-color property to transparent. I know this is not CSS, but I feel like designating background-color as null will throw many users off.

WDYT?

@ndelangen
Copy link
Member

ndelangen commented Jun 15, 2022

The type for global background will be either null or string. Is this desirable?

It's acceptable

In CSS, there's no such thing as background-color: none or background-color: null. If you want the background to have no color,

you set background-color property to transparent. I know this is not CSS, but I feel like designating background-color as null will throw many users off.
I don't think users would ever really see this.. it's an implementation detail, where the value null is an indicator to our code applying the CSS to disregard the value, and use a fallback.

@ndelangen
Copy link
Member

@kylegach @shilman What do you think we should do here?

I think we discussed maybe rebuilding backgrounds-addon?

@ndelangen ndelangen removed their assignment Jun 30, 2022
@bytrangle bytrangle closed this Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maintenance User-facing maintenance tasks
Projects
None yet
Development

Successfully merging this pull request may close these issues.

addon-backgrounds: global context not initially updated with color
4 participants