FEPLT-2075: storybook migration b4 #4447
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sarkaaa
changed the title
|
Storybook staging is available at https://kiwicom-orbit-sarka-storybook-migration-b4.surge.sh |
|
Size Change: -1 B (0%) Total Size: 461 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
340c08d
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://fa1d17ca.orbit.pages.dev |
| Branch Preview URL: | https://sarka-storybook-migration-b4.orbit.pages.dev |
sarkaaa
commented
Aug 13, 2024
|
|
||
| Neutral.story = { |
There was a problem hiding this comment.
For this Story file, there were soooo many similar stories (the differences were only in type prop), so I squash them into one story with type option.
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
5 times, most recently
from
3ecc13e
to
3b79bf6
Compare
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
3 times, most recently
from
b010e39
to
2e5ca6d
Compare
sarkaaa
changed the title
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
3 times, most recently
from
e682697
to
93bc386
Compare
sarkaaa
commented
Aug 22, 2024
| import Tooltip from "../Tooltip"; | ||
| import Tile from "../Tile"; | ||
| import Box from "../Box"; | ||
|
|
||
| import Modal, { ModalHeader, ModalSection, ModalFooter } from "."; | ||
|
|
||
| export default { | ||
| type ModalPropsAndCustomArgs = React.ComponentProps<typeof Modal> & |
There was a problem hiding this comment.
There are several ways how to improve this story file, however, this component will be implemented, so I suggest to focus more on this story once the component is refreshed. WDYT?
There was a problem hiding this comment.
I would actually maybe add more stories here (or made improvements) as it could be beneficial during the reimplementation if we already have some scenarios in mind - component might be reimplemented but the functionality and design will stay the same AFAIK.
What improvements do you have in mind?
There was a problem hiding this comment.
One of the improvements I would already suggest is to rename the stories to be more meaningful and to immediately show what the story is about - "short modal" is basically only section without header and footer, "sizes" doesn't really make sense as you can just set the size everywhere if we add the control, etc. :D
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
3 times, most recently
from
1c1332c
to
d381410
Compare
sarkaaa
commented
Aug 23, 2024
| control: { | ||
| type: "select", | ||
| parameters: { | ||
| info: "Playground of Loading component. Type buttonLoader inherits parent's stroke color. For visualisation of undefined value for customSize arg, set value number 0. Visit Orbit.Kiwi for more detailed guidelines.", |
There was a problem hiding this comment.
Some of the component props depend on the specific configuration, so I wrote some extended instructions and explanations.
sarkaaa
commented
Aug 23, 2024
| </div> | ||
| ); | ||
| }; | ||
| export const Playground: Story = { |
There was a problem hiding this comment.
I was considering squashing the Accessibility and Playground stories as they're almost the same (Accessibility includes ariaLabel attribute. What's your opinion?
There was a problem hiding this comment.
I agree. Usually (or maybe even for every component), there is more controls in the Playground story but here Accessibility story has one extra control. If we would keep the Accessibility story, it would maybe make more sense to leave there only one control - the ariaLabel one. But I would prefer just keeping Default, Playground and RTL ones.
sarkaaa
changed the title
packages/orbit-components/src/NotificationBadge/NotificationBadge.stories.tsx
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/NotificationBadge/NotificationBadge.stories.tsx
Outdated
Show resolved
Hide resolved
| info: "You can try all possible configurations of this component. However, check Orbit.Kiwi for more detailed design guidelines.", | ||
| }, | ||
| }; | ||
| export const Sizes: Story = { |
There was a problem hiding this comment.
I wouldn't remove the sections from here as now the modal doesn't have enough height to scroll and mobileHeader functionality is not visible.
| import Tooltip from "../Tooltip"; | ||
| import Tile from "../Tile"; | ||
| import Box from "../Box"; | ||
|
|
||
| import Modal, { ModalHeader, ModalSection, ModalFooter } from "."; | ||
|
|
||
| export default { | ||
| type ModalPropsAndCustomArgs = React.ComponentProps<typeof Modal> & |
There was a problem hiding this comment.
One of the improvements I would already suggest is to rename the stories to be more meaningful and to immediately show what the story is about - "short modal" is basically only section without header and footer, "sizes" doesn't really make sense as you can just set the size everywhere if we add the control, etc. :D
| Rtl.story = { | ||
| name: "RTL", | ||
| type PlagroundStory = StoryObj<ModalPropsAndCustomArgs & { header: boolean; footer: boolean }>; | ||
| export const Playground: PlagroundStory = { |
There was a problem hiding this comment.
I would add here much larger modal, so you can for example see the suppressed sections better (e.g. how it is now in the fixed footer story) and mobileHeader prop.
hasCloseButton, disableAnimation, labelClose is missing.
Maybe it would make sense to also include flex prop from Footer?
| preventOverlayClose={preventOverlayClose} | ||
| > | ||
| {header && ( | ||
| export const WithoutSection: Story = { |
There was a problem hiding this comment.
What is the difference between this story and removable section story?
There was a problem hiding this comment.
It seems there's not any significant difference, I'll remove one of these stories.
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
2 times, most recently
from
661b407
to
1d3f665
Compare
|
ok, so I updated most of the stories (except Loading for now), I create 2nd commit for Modal, which I'll squash once it will be finalized. In the modal:
|
sarkaaa
commented
Sep 3, 2024
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
3 times, most recently
from
84d3983
to
a39deb8
Compare
|
Almost everything should be fixed, except the From my research, the previous SB included custom I think we have these options:
I like the most the second option (as it's now) because we don't mix What's your opinion? @domihustinova UPDATE going this way we avoid the issue related to maintaining the enum when the palette color name list is updated |
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
2 times, most recently
from
649e143
to
9c8cec8
Compare
|
Regarding the |
packages/orbit-components/src/NotificationBadge/NotificationBadge.stories.tsx
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/ListChoice/ListChoice.stories.tsx
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/ListChoice/ListChoice.stories.tsx
Outdated
Show resolved
Hide resolved
packages/orbit-components/src/ListChoice/ListChoice.stories.tsx
Outdated
Show resolved
Hide resolved
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
2 times, most recently
from
74ac40b
to
059df01
Compare
| type: "select", | ||
| parameters: { | ||
| controls: { | ||
| exclude: ["icon", "iconColor", "children"], |
There was a problem hiding this comment.
Now when you removed these from meta args it's not necessary to exclude them, no? Same below.
| ); | ||
| }; | ||
| args: { | ||
| ...MultipleChoices.args, |
There was a problem hiding this comment.
Prop id is missing here
| args: { | ||
| ...MultipleChoices.args, | ||
| selectable: false, | ||
| selected: false, |
There was a problem hiding this comment.
This is already applied in MultipleChoices.args, no?
There was a problem hiding this comment.
yes, it is, but I want to set false value for selectable as a default value for this specific story (to show the user that the Plus icon can be used in list choice). selected can be moved out, true, I didn't notice that. 👍
There was a problem hiding this comment.
Yes, yes, selectable is true in MultipleChoices.args, I was referring only to selected 👍
| export const WithAction: Story = { | ||
| render: ({ icon, ...args }) => { | ||
| const Icon = typeof icon === "string" && getIcon(icon); | ||
| const IconButton = typeof icon === "string" && getIcon("Plus"); |
There was a problem hiding this comment.
Is the condition check necessary, can't it be just const IconButton = getIcon("Plus");? 🤔
| export const Playground = ({ type, loadingText, loading, dataTest }) => { | ||
| return <Loading loading={loading} type={type} text={loadingText} dataTest={dataTest} />; | ||
| }; | ||
| export const CardLoading: StoryObj<CardLoadingStoryProps> = { |
There was a problem hiding this comment.
CardLoadingStoryProps is now not necessary, no?
| ...DefaultWithChildren.args, | ||
| customSize: 50, | ||
| id: "loader-id", | ||
| loading: false, |
There was a problem hiding this comment.
Since you removed loading from controls (as it doesn't do anything as you said), should it be here set to false if the loader is actually loading?
| }, | ||
| }, | ||
| }; | ||
|
|
||
| export const RemovableSections: Story = { | ||
| render: ({ showSection, children, lockScrolling }) => { |
There was a problem hiding this comment.
Does the lockScrolling has any function here? It's excluded from controls so I guess it can be removed from her and line 78?
There was a problem hiding this comment.
There was a general mess with using lockScrolling, so I changed usage at multiple places. :D
| }, | ||
| }; | ||
|
|
||
| type PlagroundStoryProps = ModalPropsAndCustomArgs & { header: boolean; footer: boolean }; |
There was a problem hiding this comment.
You have a typo here in PlagroundStoryProps, also on the line 456 Itineary, can you please fix? 🙏
| }, | ||
| illustration: { | ||
| options: NAMES, |
There was a problem hiding this comment.
What do you think about adding "undefined" here as an option as well (I think I saw it in some other file) so it's possible to remove illustration as well? 🤔
There was a problem hiding this comment.
yes, why not. I did the same approach for icon prop in some other component. 👍
| parameters: { | ||
| info: "An example of a modal with a form. Check Orbit.Kiwi for more detailed design guidelines.", |
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
from
059df01
to
372663c
Compare
sarkaaa
force-pushed
the
sarka/storybook-migration-b4
branch
from
372663c
to
340c08d
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.tsCloses https://kiwicom.atlassian.net/browse/FEPLT-2075