Select knob key/value ordering #1745

psimyn · 2017-08-27T08:45:32Z

continued from #1449

fixes #799

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`
})

Is this testable with jest or storyshots?

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.

This change is

codecov · 2017-08-27T08:51:17Z

Codecov Report

Merging #1745 into master will increase coverage by 0.1%.
The diff coverage is 57.14%.

@@            Coverage Diff            @@
##           master    #1745     +/-   ##
=========================================
+ Coverage   35.73%   35.84%   +0.1%     
=========================================
  Files         428      428             
  Lines        9444     9452      +8     
  Branches      978      980      +2     
=========================================
+ Hits         3375     3388     +13     
+ Misses       5421     5405     -16     
- Partials      648      659     +11

Impacted Files	Coverage Δ
addons/knobs/src/angular/index.js	`0% <ø> (ø)`	⬆️
addons/knobs/src/vue/index.js	`24.13% <ø> (ø)`	⬆️
addons/knobs/src/index.js	`0% <ø> (ø)`	⬆️
addons/knobs/src/react/index.js	`40% <ø> (ø)`	⬆️
addons/knobs/src/base.js	`11.76% <33.33%> (+5.09%)`	⬆️
addons/knobs/src/components/types/Select.js	`24.63% <63.63%> (+16.7%)`	⬆️
app/react/src/server/utils.js	`0% <0%> (-53.58%)`	⬇️
lib/core/src/client/preview/story_store.js	`84.21% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/components/menu_item.js	`19.14% <0%> (ø)`	⬆️
addons/knobs/src/components/types/Button.js	`11.9% <0%> (ø)`	⬆️
... and 54 more

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 2d8db3e...5928842. Read the comment docs.

danielduan · 2017-08-28T18:50:01Z

Think this should go out as a minor version instead of a patch.

Is this documented anywhere and also is there an example somewhere in the cra-kitchen-sink for this?

psimyn · 2017-09-10T12:33:59Z

@danielduan I've updated addons README (and rebased on latest release/3.3)

Not sure about adding to kitchen-sink examples, since for users it is identical to existing select. What do you think?

danielduan · 2017-11-15T18:54:02Z

addons/knobs/README.md

+};
+const defaultValue = 'Red';
+
+const value = select(label, options, defaultValue);


effectively, this should be selectV2() with this PR change?

I don't think this alleviates the breaking change, it only defers the problem to a later date.

yes, seems that by doing that we could release this early and deprecate the old method, and then remove the old implementation at the next major.

yep, this means the breaking change in v4 will be renaming selectV2 to select

this was meant to be selectV2, I've updated it 👍

ndelangen · 2017-11-15T20:53:35Z

@psimyn The addon has changed somewhat since this PR was rebased, mainly support for Vue and Angular was introduced, and thus a lot of code was moved.

Do you think you could fix the merge conflicts? Let me know if you need help?

fixes #799 add selectV2 for backwards compatibility

psimyn · 2017-12-30T00:08:51Z

@ndelangen I've updated this with latest master, and added a test for the SelectType component.

Let me know if anything else is needed - the existing example is still valid (since it uses the Array of options).

apologies for the massive delay!

Hypnosphi · 2018-01-01T22:27:02Z

Should we add a deprecation notice for old select usages?

ndelangen · 2018-01-02T13:23:43Z

@psimyn Thanks for the hard work! Really appreciate the time and effort you've put in!

This is a good step overall to improve the knobs API!

I feel it's merge-able without the deprecation warning on select, but if you feel like adding it to this PR that's great!

Merge when ready IMHO

psimyn · 2018-01-02T20:47:56Z

thanks @ndelangen & @Hypnosphi

I'll add deprecate, but could use some help with wording. Currently have:

select: 'in v4 the options keys/values are being swapped'
selectV2: 'in v4 this will be renamed to select'

ndelangen · 2018-01-02T22:10:28Z

@shilman You wanna pitch on on the deprecation messages?

…knob-ordering

…k into select-knob-ordering

Hypnosphi · 2018-01-15T13:13:05Z

select: 'in v4 the options keys/values are being swapped'
selectV2: 'in v4 this will be renamed to select'

The problem with this is that currently there is no way for a user to avoid getting warnings. Maybe we should add the latter warning only in 4.0, and remove selectV2 only in 5.0

Hypnosphi · 2018-01-15T13:15:18Z

@kevinSuttle in your case, keys and values are the same string, so this PR shouldn't change anything for you.

Btw, you can use an array for that:

const buttonVariants = [
  "primary",
  "secondary",
  "negative",
  "alternate"
];

psimyn · 2018-01-18T06:53:36Z

thanks @Hypnosphi - I didn't like always showing deprecation message. I've removed the v2 one

Hypnosphi · 2018-01-18T10:23:33Z

addons/knobs/src/base.js

+    manager.knob(name, { type: 'select', options, value }),
+    'in v4 keys/values of the options argument are reversed'
+  );
+}


For some reason, it throws in unit tests:

TypeError: Object prototype may only be an Object or null: apple at Function.setPrototypeOf (<anonymous>) 74 | 75 | function select(name, options, value) { > 76 | return (0, _utilDeprecate2.default)(manager.knob(name, { type: 'select', options: options, value: value }), 'in v4 keys/values of the options argument are reversed'); 77 | } 78 | 79 | function selectV2(name, options, value) { at select (addons/knobs/dist/base.js:76:38)

https://circleci.com/gh/storybooks/storybook/33655?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

I was doing it wrong :)

fixed this up and added selectV2 example

Hypnosphi

Please add some selectV2 usages to examles/official-storybook

…knob-ordering

…k into select-knob-ordering

ndelangen · 2018-01-31T21:04:26Z

Perfect @psimyn This is good to go IMHO 👍

ndelangen · 2018-01-31T21:07:04Z

Is there something we're doing wrong that is causing the deprecation message to appear multiple times?

Hypnosphi · 2018-01-31T22:09:39Z

addons/knobs/src/base.js

+    manager.knob.bind(manager),
+    'in v4 keys/values of the options argument are reversed'
+  )(name, { type: 'select', options, value });
+}


This should probably be rewritten as follows:

export const select = deprecate( (name, options, value) => manager.knob(name, { type: 'select', options, value }), 'in v4 keys/values of the options argument are reversed' );

See #1745 (comment)

thanks! I was missing the arrow function before and passing the result of manager.knob 🤦

…k into select-knob-ordering

psimyn · 2018-01-31T22:55:59Z

thanks @ndelangen & @Hypnosphi

multiple warnings was due to me deprecating incorrectly. Fixed up now 👍

psimyn mentioned this pull request Aug 27, 2017

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

Closed

danielduan added addon: knobs BREAKING CHANGE labels Aug 28, 2017

danielduan changed the base branch from master to release/3.3 August 28, 2017 18:47

psimyn force-pushed the select-knob-ordering branch from 54c09a8 to 7801d5a Compare September 10, 2017 12:24

psimyn removed the BREAKING CHANGE label Oct 16, 2017

danielduan requested review from ndelangen, shilman and Hypnosphi November 15, 2017 18:50

danielduan reviewed Nov 15, 2017

View reviewed changes

ndelangen mentioned this pull request Nov 15, 2017

select knob seems backwards #799

Closed

Hypnosphi changed the base branch from release/3.3 to master December 23, 2017 23:55

psimyn force-pushed the select-knob-ordering branch from 7801d5a to 81deff8 Compare December 29, 2017 23:49

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

a12d5c5

fixes #799 add selectV2 for backwards compatibility

psimyn force-pushed the select-knob-ordering branch from 993f872 to a12d5c5 Compare December 29, 2017 23:55

ndelangen added 2 commits January 1, 2018 23:02

Merge branch 'master' into select-knob-ordering

4791306

Update README.md

c19a445

ndelangen approved these changes Jan 1, 2018

View reviewed changes

Hypnosphi added the feature request label Jan 1, 2018

Hypnosphi assigned psimyn Jan 2, 2018

Merge branch 'master' into select-knob-ordering

68b543a

psimyn and others added 4 commits January 15, 2018 09:32

Merge branch 'master' into select-knob-ordering

0605b99

Merge branch 'master' of github.com:storybooks/storybook into select-…

320a3d8

…knob-ordering

Merge branch 'select-knob-ordering' of github.com:storybooks/storyboo…

92a68f0

…k into select-knob-ordering

add deprecation messages for select and selectV2

81864f8

psimyn and others added 2 commits January 18, 2018 17:51

don't deprecate selectV2 until v5

571069b

Merge branch 'master' into select-knob-ordering

475dae7

Hypnosphi approved these changes Jan 18, 2018

View reviewed changes

Hypnosphi reviewed Jan 18, 2018

View reviewed changes

Hypnosphi requested changes Jan 18, 2018

View reviewed changes

psimyn and others added 7 commits January 24, 2018 14:26

Merge branch 'master' of github.com:storybooks/storybook into select-…

a188468

…knob-ordering

Merge branch 'master' of github.com:storybooks/storybook into select-…

880069d

…knob-ordering

fix deprecate message, add example for select v2

c0c8f3c

Merge branch 'select-knob-ordering' of github.com:storybooks/storyboo…

b3e785e

…k into select-knob-ordering

fix example

1d1f0a7

update snapshot with text from selectV2 example

1fa81fb

Merge branch 'master' into select-knob-ordering

b2f22ae

Hypnosphi requested changes Jan 31, 2018

View reviewed changes

psimyn added 2 commits February 1, 2018 09:53

deprecate properly

0a1140f

Merge branch 'select-knob-ordering' of github.com:storybooks/storyboo…

5928842

…k into select-knob-ordering

Hypnosphi approved these changes Jan 31, 2018

View reviewed changes

ndelangen merged commit 7b0b539 into master Jan 31, 2018

ndelangen deleted the select-knob-ordering branch January 31, 2018 23:08

diagramatics mentioned this pull request Aug 3, 2018

Add selectV2 definition to @storybook/addon-knobs DefinitelyTyped/DefinitelyTyped#27848

Merged

9 tasks

dependabot-preview bot mentioned this pull request Oct 18, 2018

Bump svelte from 2.13.5 to 2.14.0 #4478

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Select knob key/value ordering #1745

Select knob key/value ordering #1745

psimyn commented Aug 27, 2017 •

edited by ndelangen

Loading

codecov bot commented Aug 27, 2017 •

edited

Loading

danielduan commented Aug 28, 2017

psimyn commented Sep 10, 2017

danielduan Nov 15, 2017

ndelangen Nov 15, 2017

psimyn Dec 29, 2017

ndelangen commented Nov 15, 2017

psimyn commented Dec 30, 2017

Hypnosphi commented Jan 1, 2018

ndelangen commented Jan 2, 2018

psimyn commented Jan 2, 2018

ndelangen commented Jan 2, 2018

Hypnosphi commented Jan 15, 2018 •

edited

Loading

Hypnosphi commented Jan 15, 2018

psimyn commented Jan 18, 2018

Hypnosphi Jan 18, 2018

psimyn Jan 30, 2018

Hypnosphi left a comment

ndelangen commented Jan 31, 2018

ndelangen commented Jan 31, 2018

Hypnosphi Jan 31, 2018

psimyn Jan 31, 2018

psimyn commented Jan 31, 2018

Select knob key/value ordering #1745

Select knob key/value ordering #1745

Conversation

psimyn commented Aug 27, 2017 • edited by ndelangen Loading

What I did

How to test

codecov bot commented Aug 27, 2017 • edited Loading

Codecov Report

danielduan commented Aug 28, 2017

psimyn commented Sep 10, 2017

danielduan Nov 15, 2017

Choose a reason for hiding this comment

ndelangen Nov 15, 2017

Choose a reason for hiding this comment

psimyn Dec 29, 2017

Choose a reason for hiding this comment

ndelangen commented Nov 15, 2017

psimyn commented Dec 30, 2017

Hypnosphi commented Jan 1, 2018

ndelangen commented Jan 2, 2018

psimyn commented Jan 2, 2018

ndelangen commented Jan 2, 2018

Hypnosphi commented Jan 15, 2018 • edited Loading

Hypnosphi commented Jan 15, 2018

psimyn commented Jan 18, 2018

Hypnosphi Jan 18, 2018

Choose a reason for hiding this comment

psimyn Jan 30, 2018

Choose a reason for hiding this comment

Hypnosphi left a comment

Choose a reason for hiding this comment

ndelangen commented Jan 31, 2018

ndelangen commented Jan 31, 2018

Hypnosphi Jan 31, 2018

Choose a reason for hiding this comment

psimyn Jan 31, 2018

Choose a reason for hiding this comment

psimyn commented Jan 31, 2018

psimyn commented Aug 27, 2017 •

edited by ndelangen

Loading

codecov bot commented Aug 27, 2017 •

edited

Loading

Hypnosphi commented Jan 15, 2018 •

edited

Loading