refactor: [M3-6522] - Toggle component v7 story migration and cleanup#9905
refactor: [M3-6522] - Toggle component v7 story migration and cleanup#9905coliu-akamai merged 7 commits intolinode:developfrom
Conversation
There was a problem hiding this comment.
We could move packages/manager/src/components/Toggle/Toggle.tsx to packages/manager/src/components/Toggle.tsx to simplify things a bit, but this is optional.
There was a problem hiding this comment.
👀 In general is there a rule of thumb for the min. amount of files that should be in a package for organization? There're currently three toggle related files - the tests, story, and toggle component itself - in the package. Would it be too much clutter if I got rid of the Toggle package?
There was a problem hiding this comment.
I personally don't mind if the src/components folder is massive but it's definitely fair to keep the src/components/Toggle folder around if there is a few files
There was a problem hiding this comment.
I like it in its own directory - makes it easier at a glance to see how things are organized and if for example a component is missing a test, a story etc. It's a bit more portable too, even if that does not matter now. Has the drawback of the import being longer but not a big deal.
coliu-akamai
requested review from
carrillo-erik and
mjac0bs
and removed request for
a team
abailly-akamai
left a comment
abailly-akamai
left a comment
There was a problem hiding this comment.
Looks good! Approving pending adding args to the story
- story and self documentation looks great ✅
- tests ✅
- no visual regression or missing import ✅
There was a problem hiding this comment.
I like it in its own directory - makes it easier at a glance to see how things are organized and if for example a component is missing a test, a story etc. It's a bit more portable too, even if that does not matter now. Has the drawback of the import being longer but not a big deal.
|
|
||
| const meta: Meta<ToggleProps> = { | ||
| component: Toggle, | ||
| title: 'Components/Toggle', |
There was a problem hiding this comment.
define your args with defaults here so we can interact with the component directly in the storybook docs. doesn't have to be all the props, but at least the ones we'd one to play with when looking at the component
mjac0bs
left a comment
mjac0bs
left a comment
There was a problem hiding this comment.
Aside from Alban's feedback and a couple very minor nitpicks, the story looks good!
Also: latest e2e run looked fine. resize-linode spec failure was not related to these changes.
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
4 participants
Description 📝
Small PR to migrate Toggle component story to a V7 (.tsx) story!
Changes 🔄
List any change relevant to the reviewer.
How to test 🧪
Prerequisites
Verification steps
As an Author I have considered 🤔
Check all that apply