Skip to content
This repository has been archived by the owner on May 2, 2023. It is now read-only.

feat(tag): Improve knobs on component - FRONT-492 #334

Merged
merged 8 commits into from
Feb 17, 2020
Merged

feat(tag): Improve knobs on component - FRONT-492 #334

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d765f67
feat(tag): Improve knobs on component - FRONT-492
Joosthe Feb 11, 2020
53627aa
feat(language-list): added quotes - FRONT-492
Joosthe Feb 12, 2020
50891e2
feat(tag): added quotes - FRONT-492
Joosthe Feb 12, 2020
86ca246
feat(tag): changed the label - FRONT-492
Joosthe Feb 12, 2020
f0094ab
Merge branch 'develop' into FRONT-492
Joosthe Feb 13, 2020
a49fc97
feat(tag): remove variants - FRONT-492
Joosthe Feb 13, 2020
6fd20fb
Merge branch 'develop' into FRONT-492
planctus Feb 13, 2020
bccd212
Merge branch 'develop' into FRONT-492
Joosthe Feb 14, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion src/ec/packages/ec-component-tag/tag.story.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import merge from 'deepmerge';
import { storiesOf } from '@storybook/html';
import { withKnobs, text } from '@storybook/addon-knobs';
import { withKnobs, text, select } from '@storybook/addon-knobs';
import { withNotes } from '@ecl-twig/storybook-addon-notes';
import withCode from '@ecl-twig/storybook-addon-code';

Expand All @@ -11,6 +11,12 @@ import dataRemovable from './demo/data--removable';
import tag from './ecl-tag.html.twig';
import notes from './README.md';

const variants = {
'As a link': 'link',
'As a Button': 'button',
Removable: 'removable',
Copy link
Contributor

Choose a reason for hiding this comment

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

missing quotes to Removable?

};

storiesOf('Components/Tag', module)
.addDecorator(withKnobs)
.addDecorator(withNotes)
Expand All @@ -23,7 +29,9 @@ storiesOf('Components/Tag', module)
tag: {
label: text('Label', 'Link tag'),
path: text('Url', '/example'),
type: select('Variable', variants, 'link'),
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose of having variants types as knobs if it's already defined as stories?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was literally the scope of the ticket to build in this functionality.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @papegaill , thanks for the review, your concern is clearly reasonable, this new select will let you switch between the different variants you have for the component which are indeed matching the stories we demo.
But if i have to consider what is redundant here, i'd say it's the "multiplication" of the stories for something that is actually just a parameter switch, not the availability of the parameter really handling the switch.
At the same time, i see that sometimes stories are a more evident way to show the different "variants" available per component, but we have to make it clear what is the real difference between them.
The label should be "variant", though, for this to happen, "variable" is not really identifying what the parameter is about

Copy link
Contributor

Choose a reason for hiding this comment

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

@planctus I think it's confusing, because for example if on the "as a link" story i switch variant, I'm then able to set "url" on a button or a removable tag. same with parameter "aria-label" available only on removable tag.

Copy link
Contributor

Choose a reason for hiding this comment

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

ok, you convinced me, let's remove it ;)

},
default_icon_path: defaultSprite,
})
),
{
Expand All @@ -37,6 +45,7 @@ storiesOf('Components/Tag', module)
merge(dataButton, {
tag: {
label: text('Label', 'Button tag'),
type: select('Variable', variants, 'button'),
},
})
),
Expand All @@ -51,6 +60,8 @@ storiesOf('Components/Tag', module)
merge(dataRemovable, {
tag: {
label: text('Label', 'Removable tag'),
type: select('Variable', variants, 'removable'),
aria_label: text('Aria label', 'Dismiss'),
},
default_icon_path: defaultSprite,
})
Expand Down