feat(tag): Improve knobs on component - FRONT-492 #334
Conversation
Joosthe
added
pr: review needed
| const variants = { | ||
| 'As a link': 'link', | ||
| 'As a Button': 'button', | ||
| Removable: 'removable', |
There was a problem hiding this comment.
missing quotes to Removable?
| @@ -23,7 +29,9 @@ storiesOf('Components/Tag', module) | |||
| tag: { | |||
| label: text('Label', 'Link tag'), | |||
| path: text('Url', '/example'), | |||
| type: select('Variable', variants, 'link'), | |||
There was a problem hiding this comment.
What's the purpose of having variants types as knobs if it's already defined as stories?
There was a problem hiding this comment.
This was literally the scope of the ticket to build in this functionality.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
ok, you convinced me, let's remove it ;)
papegaill
added
pr: modification needed
and removed
pr: review needed
Joosthe
added
pr: review needed
| label: text('Label', 'Link tag'), | ||
| path: text('Url', '/example'), | ||
| label: text('Label', dataLink.tag.label), | ||
| path: text('Url', dataLink.tag.path), |
There was a problem hiding this comment.
MMhh, but if we are doing this in order to match as much as possible the specs, why we name the knob "url" when the property passed to the template is path? I understand that in any case a bit of ambiguity is in place, considering that both a path and an url can be used, so that's what i would do, we name it: Path (accepts also a full url), what do you think?
planctus
added
pr: questions
and removed
pr: review needed
PR description
Please drop a few lines about the PR: what it does, how to test it, etc.
QA Checklist
In order to ensure a safe and quick review, please check that your PR follow those guidelines:
devDependenciesdevDependenciesincludeorembed) asdependencies@ecl-twig/ec-components'sdependenciesyarn.lockcontains changes, I have committed itpr: review neededto indicate that I'm done and now waiting for a review ,pr: wipto indicate that I'm actively working on it ...)