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

[Backgrounds addon] Set a different default background for a specific story #9164

Closed
iansan5653 opened this issue Dec 16, 2019 · 14 comments
Closed

Comments

@iansan5653
Copy link

Is your feature request related to a problem? Please describe.
I have a component prop that controls whether the component is visible on a dark background or a light background. In the dark-background story, I'd like to default to the dark background by name, rather than overriding all the backgrounds for that component.

Describe the solution you'd like
Something like:

storiesOf("Logo", module)
  .add("horizontal", () => <Logo layout="horizontal" />)
  .add("vertical", () => <Logo layout="vertical" />)
  .add("horizontal - inverted", () => <Logo layout="horizontal" inverted/>, {defaultBackground: "primary"})
  .add("vertical - inverted", () => <Logo layout="vertical" inverted/>, {defaultBackground: "primary"});

Describe alternatives you've considered
I saw on the documentation page that you can override the background for a component, but I don't see a way to leave the other options and just change which one is the default. It may be possible, but I don't see how in the documentation.

Are you able to assist bring the feature to reality?
Maybe; I'm not sure if I can.

@shilman
Copy link
Member

shilman commented Dec 18, 2019

@kevin940726 this seems related to your change in #9137, but for backgrounds instead of viewports. any chance you can take a quick look?

@kevin940726
Copy link
Contributor

Sure, I'll take a look. Thx for asking!

@kevin940726
Copy link
Contributor

kevin940726 commented Dec 19, 2019

We can support this by accepting a new field defaultBackground in the background parameter config.

Overridden.story = {
  parameters: {
    backgrounds: { defaultBackground: "primary" }
  }
};

The API is intended for the users to override the backgrounds, but I agree that it's somewhat annoying to do as a regular basis.

IMHO the current config API is pretty simple, and thus not so powerful and error-prone. If we want to provide more control and flexibility, we might want to consider a different data structure? Maybe a normalized object with an ordered list. (In addition, maybe the order doesn't really matter? Or we can pre-sort the order for the users? based on brightness or something like that.)

Needless to say, there would probably be breaking changes. I can help make this (defaultBackground) happen first though, please let me know what you think about this :)

@iansan5653
Copy link
Author

The defaultBackground method looks perfect! It's exactly what I was thinking of.

@kevin940726
Copy link
Contributor

Here's some updates after digging into the source a little bit more. Turns out that the parameters value is merged upon creation, so we cannot get the parents' parameters if they've been overridden. It's a fundamental design choice, so it's not easy to fix. (Correct me if I was wrong though!)

For now the best way to work around this issue is to define a global variable and a marker function to mark the default.

const BACKGROUNDS = [
  { name: "white", value: "#ffffff" },
  { name: "light", value: "#eeeeee" },
  { name: "gray", value: "#cccccc" },
  { name: "dark", value: "#222222" },
  { name: "black", value: "#000000" }
];

function markDefault(backgrounds, defaultBackground) {
  // make a copy
  const newBackgrounds = backgrounds.map(bg => ({ ...bg }));

  const defaultBg = newBackgrounds.find(bg => bg.name === defaultBackground);
  defaultBg.default = true;

  return newBackgrounds;
}

export default {
  parameters: {
    backgrounds: markDefault(BACKGROUNDS, "dark")
  }
};

Story1.story = {
  parameters: {
    backgrounds: markDefault(BACKGROUNDS, "light")
  }
};

It's not ideal, but it's the only way I can think of with current limitations.

We could provide a different parameter name like defaultBackground, but I doubt that it would be a better idea?

Story1.story = {
  parameters: {
    defaultBackground: "light"
    // instead of backgrounds: {  defaultBackground: 'light'  }
  }
};

@shilman
Copy link
Member

shilman commented Dec 22, 2019

Thanks @kevin940726. The way we handle parameters is actually up for grabs. Just filed #9219. It doesn't solve this problem directly, but you can see how it would be much easier to solve if the parameters were shallow-merged.

cc @ndelangen @tmeasday

@shilman
Copy link
Member

shilman commented Dec 23, 2019

Turns out that #9219 doesn't really apply. Here's what I'm thinking for a better backgrounds API. We can deprecate the old API in 6.0 and remove it in 7.0:

// preview.js
addParameters({
  backgrounds: {
    default: 'white',
    values: [
      { name: 'white', value: '#ffffff' }, 
      { name: 'black', value: '#000000' },
    ]
  }
}

// Foo.stories.js
export const Foo = ...
Foo.story = {
  parameters: {
    backgrounds: { default: 'black' },
  }
}

@tmeasday
Copy link
Member

Yeah perhaps this is a better solution in the 6.0 timeframe than my plan :)

@tmeasday
Copy link
Member

tmeasday commented Dec 23, 2019

Although we are going to to run into the problem that @kylesuss was debugging in chromaui/chromatic-cli#80, and see backgrounds.values get concat-ed if you try and overwrite it :/

I feel like we should drop that behaviour, I wonder why we added it?

Update: it looks like @ndelangen added it in order to fix some tests: deade9d

I guess if we remove it, we can see which tests break to understand why ;)

@shilman
Copy link
Member

shilman commented Dec 23, 2019

Yeah I think concatenating the values is not a desirable behavior. @ndelangen what's the use case? I propose we revert this in 6.0.

@ndelangen
Copy link
Member

I agree we have a clearer picture of the best practices now, and should form an opinion on the dataformat inside parameters.

addParameters({
  backgrounds: {
    default: 'white',
    values: [
      { name: 'white', value: '#ffffff' }, 
      { name: 'black', value: '#000000' },
    ]
  }
}

Looks like an improvement to me!

@stale
Copy link

stale bot commented Jan 13, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@stale stale bot added the inactive label Jan 13, 2020
@shilman shilman added the maintenance User-facing maintenance tasks label Jan 16, 2020
@stale stale bot removed the inactive label Jan 16, 2020
@stale
Copy link

stale bot commented Feb 6, 2020

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 30 days. Thanks!

@shilman
Copy link
Member

shilman commented Apr 28, 2020

Re-opening as #10572

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants