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.

Sign up for GitHub

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

Jump to bottom

Removed lowercase-only restriction for knobs #2646

Merged
merged 5 commits into from
Jan 5, 2018
Merged

Removed lowercase-only restriction for knobs #2646

merged 5 commits into from
Jan 5, 2018

Conversation

alterx
Copy link
Member

@alterx alterx commented Jan 4, 2018

Take in account that now the knob name needs to be the same as the angular component @input attribute. Otherwise it'll not bind to the correct property.

Issue: #2641

What I did

Removed lowercase-only restriction for knobs

How to test

yarn bootstrap --reset --core
cd examples/angular-cli
yarn storybook
open http://localhost:9009

@alterx
Removing lowercase-only restriction for knobs
0c47992 
Take in account that now the knob name needs to be the same as the angular component @input attribute. Otherwise it'll not bind to the correct property.

Signed-off-by: Carlos Vega <[email protected]>
@alterx alterx added the angular label Jan 4, 2018
@codecov
Copy link

codecov bot commented Jan 4, 2018 •
edited
Loading

Codecov Report

Merging #2646 into master will increase coverage by 0.27%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2646      +/-   ##
==========================================
+ Coverage   34.06%   34.34%   +0.27%     
==========================================
  Files         383      389       +6     
  Lines        8710     8750      +40     
  Branches      897      911      +14     
==========================================
+ Hits         2967     3005      +38     
- Misses       5148     5150       +2     
  Partials      595      595
Impacted Files Coverage Δ
addons/knobs/src/angular/helpers.js 0% <0%> (ø) ⬆️
app/vue/src/server/config/babel.js 0% <0%> (-100%) ⬇️
app/vue/src/server/utils.js 0% <0%> (-53.58%) ⬇️
...es__/update-addon-info/update-addon-info.output.js 0% <0%> (ø) ⬆️
addons/jest/src/components/Result.js 0% <0%> (ø) ⬆️
addons/a11y/src/components/Report/index.js 0% <0%> (ø) ⬆️
addons/actions/src/lib/reviver.js 58.33% <0%> (ø) ⬆️
addons/actions/src/lib/util/getPropertiesList.js 60% <0%> (ø) ⬆️
addons/graphql/src/components/FullScreen/index.js 0% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
... and 56 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 3515d04...07c97ad. Read the comment docs.

@Hypnosphi Hypnosphi added bug patch:yes Bugfix & documentation PR that need to be picked to main branch labels Jan 4, 2018
alterx added 2 commits January 4, 2018 10:49
@alterx
Removing lowercase-only restriction for knobs
d129d65 
Take in account that now the knob name needs to be the same as the angular component @input attribute. Otherwise it'll not bind to the correct property.

Signed-off-by: Carlos Vega <[email protected]>
@alterx
Merge branch 'camelcased-angular-knobs' of github.com:storybooks/stor…
50657f5 
…ybook into camelcased-angular-knobs
@alterx alterx requested a review from igor-dv January 4, 2018 17:04
@alterx alterx self-assigned this Jan 4, 2018
igor-dv
igor-dv approved these changes Jan 4, 2018
View reviewed changes
addons/knobs/src/angular/helpers.js
@@ -73,11 +73,10 @@ const getAnnotatedComponent = ({ componentMeta, component, params, knobStore, ch
const oldValue = knobOptions.value;
knobOptions.value = value;
knobStore.markAllUnused();
const lowercasedName = name.toLocaleLowerCase();
this[lowercasedName] = value;
this[name] = value;
Copy link
Member

Choose a reason for hiding this comment

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

So as I understand we can't give a different name to knobs? It has to be a name of the prop, right? Or maybe all knobs are working the same way and I missing something?

Copy link
Member Author

@alterx alterx Jan 5, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Exactly. The knobs work a bit different in the other frameworks because in those you can just send the value in as a prop. With Angular we need to bind to an specific field and check for the changes. That's why, in this specific case, to preserve the same api the name of the knob needs to be equal to the field name.

@alterx alterx merged commit 994539d into master Jan 5, 2018
@alterx alterx deleted the camelcased-angular-knobs branch January 5, 2018 17:03
@shilman shilman mentioned this pull request Jan 7, 2018
Merged
shilman pushed a commit that referenced this pull request Jan 7, 2018
@alterx @shilman
Merge pull request #2646 from storybooks/camelcased-angular-knobs
70d56d1 
Removed lowercase-only restriction for knobs
@Hypnosphi Hypnosphi added the patch:done Patch/release PRs already cherry-picked to main/release branch label Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@igor-dv igor-dv igor-dv approved these changes

Assignees

@alterx alterx

Labels
angular 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
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@alterx @igor-dv @Hypnosphi