WIP: key and value for select knob match key/value of options

[psimyn]

fixes #799

Breaking change

Issue:

Using knobs is currently confusing.

Example, for complex strings and null values

select('My Select', {
  none: null, // renders empty item
  [`Computed key`]: 'computed', 
  'Some kind of long sentence. This is ok, mainly just looks odd as a key': 'sentence',
})

What I did

Use value as value
Still uses key for the key property on each <option>

How to test

select('My Select', {
  Empty: null,
  'First item': 'The first item selected',
  second: `The item with key 'second' is selected`
})

Will add some tests. Is there a preference between __tests__ folder and *.test.js?

[codecov]

Codecov Report

❗ No coverage uploaded for pull request base (master@5681c40). Click here to learn what that means.
The diff coverage is 0%.

@@            Coverage Diff            @@
##             master    #1449   +/-   ##
=========================================
  Coverage          ?   20.69%           
=========================================
  Files             ?      249           
  Lines             ?     5585           
  Branches          ?      666           
=========================================
  Hits              ?     1156           
  Misses            ?     3933           
  Partials          ?      496

Impacted Files	Coverage Δ
addons/knobs/src/components/types/Select.js	8.19% <0%> (ø)

---

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 5681c40...4ee6039. Read the comment docs.

[shilman]

@psimyn @ndelangen how do we want to release this breaking change?

• keep the PR open until we hit 4.0?
• sneak it in for 3.2?
• sneak it in for 3.1.x?

[psimyn]

@shilman I'd like it in stable as soon as possible. On the fence about breaking semver, I'm not using knobs addon for anything critical. I addon package are supposed to stay at same version as main repo?

[ndelangen]

We release all packages with the same major.

This IS a breaking api change. this SHOULD be a major version.

BUT keeping this from users until 4.0.0 seems ... meh

Can we think about toggling this behavior somehow?

[shilman]

@psimyn regarding stability, it's about the thousands of companies and developers using storybook, not whether you're using it for anything critical 😉

I propose we put it into 3.2, which should be a pretty good release. It's not strict semver, but at least people will be getting something for upgrading with a breaking change.

Another option would be to fix some stuff with the addons API and rename 3.2 to 4.0. Not ideal, but especially if we could get Angular2 in there, it would be a great release!

[igor-dv]

What about to backward-compatible it ? For example crate a new Select_4_0.js (or some better name =/ ) that will include the fix.. And in a major release (4.0) to break it..

[shilman]

@igor-dv can you explain in more detail what you are thinking? So we put this into a Select_next.js and then enable it using some kind of feature flag setOptions({ knobSelectDisplayValue: true }) until 4.0 when we make that the default?

[igor-dv]

@shilman , exactly, I couldn't describe it better ;)

[psimyn]

Would it be ok to override for each select?.

--- a/addons/knobs/src/index.js
+++ b/addons/knobs/src/index.js
@@ -42,8 +42,8 @@ export function object(name, value) {
   return manager.knob(name, { type: 'object', value });
 }

-export function select(name, options, value) {
-  return manager.knob(name, { type: 'select', options, value });
+export function select(name, options, value, displayValue = false) {
+  return manager.knob(name, { type: 'select', options, value, displayValue });
 }

@igor-dv @shilman if global setting is better can you please point me to somewhere else that does it? I wasn't sure where to store global options

[ndelangen]

Good work @psimyn !!!

My first idea would be to export an additional selectV2 function.
Obviously super dirty, but switching implementation at runtime is worse IMHO.

[ndelangen]

This would be fine for 4.0.0 but that's some ways off I think.

Like I suggested, let's find a way to support both until 4.0.0!

[EloB]

Do I understand this PR correctly. Will this be possible after this gets merged?

const SomeComponent = ({ icon: Icon }) => (
  <div className="some-component">
    {Icon && <Icon className="some-component__icon" />}
  </div>
);

const MailIcon = props => <img {...props} src="/some/path/mail.png" />;
const LockIcon = props => <img {...props} src="/some/path/lock.png" />;

<SomeComponent
  icon={select('Icon', {
    "None": null,
    "Mail": MailIcon, // React component
    "Lock": LockIcon, // React component
  })}
/>

If so when will this be added? It's a really good feature! ❤️

[EloB]

Just figured out how todo the same behaviour right now.

const icons = {
  None: null,
  MailIcon,
  LockIcon,
};

<SomeComponent
  icon={icons[select('Icon', Object.keys(icons)]}
/>

[Hypnosphi]

What we can do without breaking (almost) anything is support an "array of objects" syntax, which is more verbose, but also more explicit:

[
  {
    label: 'None',
    value: null
  },
  {
    label: 'Mail',
    value: MailIcon
  }
]

[ndelangen]

Code LGTM,

We need to agree on a release strategy

[psimyn]

I had some craziness happen with remotes, so I created #1745

Adds selectV2 until we make it default