commercetools / ui-kit

Component library 💅
https://uikit.commercetools.com
MIT License
145 stars 25 forks source link

Storybook v8 stories batch 3 #2879

Closed misama-ct closed 2 months ago

misama-ct commented 2 months ago

Background

In an effort to switch from Storybook v5 to Storybook v8, the existing stories had to be migrated to a new storybook-format and converted to typescript.

This pull-request is part of a batch. It contains the typescript-equivalents of existing storybook v5 stories and aims to replicate the same functionality / value.

Goal

Feature parity between v8 and v5 storybook. After merging all batches, a product developer who looked at the storybook v5 version yesterday, should be able to look at the v8 version tomorrow and be as productive (or more productive) as before.

Review instructions

A thorough code review is not required yet. The code was either copied from the existing js-equivalent or quickly written from scratch to replicate what was already there to fit the new story-format. It is expected that those stories contain typescript errors, introduced by the conversion or previously existing. (Another pull-request will take care of those issues at a later stage).

What is expect is a story review:

You will need to compare the v5 with the v8 storybook (ideally side-by-side) and make sure that every component in this pull-request has:

You will find the links to the different storybooks below.

If something is missing or irritating, add a comment to the source file here in the pull-request to start a conversation.

changeset-bot[bot] commented 2 months ago

⚠️ No Changeset found

Latest commit: 8ac69deccffa162286c3147eff8b5f6a9d27e6b3

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

vercel[bot] commented 2 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
ui-kit ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 9:47am
ui-kit-storybook-v8 ✅ Ready (Inspect) Visit Preview 💬 Add feedback Aug 12, 2024 9:47am
ddouglasz commented 2 months ago

Awesome work! Looks great and consistent with our components in V5. Please correct me if I missed a strategy but if this PR is targeted at the Fields component, then we are missing the following components:

Are we assuming we would not be needing them since they seem to be consuming the same input field and have somewhat the same behaviour ?

Thank you again!

ddouglasz commented 2 months ago

README (DateField): Signature onInfoButtonClick section is missing event parameter. V5

image

V8

image
ddouglasz commented 2 months ago

DateField basic example seems to be broken when hintIcon knob is toggled: 2024-08-09 17 09 46

ddouglasz commented 2 months ago

LocalizedTextField has the exact issue too as the date field hintIcon problem.

ddouglasz commented 2 months ago

MoneyFIeld:

  1. The currency selection is supposed to be a dropdown by default, unless when it the currency is disabled. V5

    image

    V8

    image
    1. When the "choose currency" knob is used, the app breaks.

2024-08-09 17 43 48

misama-ct commented 2 months ago

[...] if this PR is targeted at the Fields component, then we are missing the following components: [..]

No strategy. I merely split up my previous working branch into several more "reviewable" chunks.

[!TIP] I initially created a pull-request for this working-branch, so that I see if CI/CD is still working as expected. On the PR page you will find links to v5 & v8 storybooks containing all stories. Of course it's getting more stale by the day, but essentially: If you can find a component there, it's in one of the batches.

misama-ct commented 2 months ago

[..] I was thinking the hintIcon knob would be a dropdown with icons lists to select from.

It should be, but one has to configure it manually unfortunately. I'll fix this.

misama-ct commented 2 months ago

README (DateField): Signature onInfoButtonClick section is missing event parameter. V5 image

V8 image

Good catch. It was an issue with the typescript annotation though and already present in main. I fixed it anyway.

misama-ct commented 2 months ago

MoneyFIeld

MoneyFIeld: 1. The currency selection is supposed to be a dropdown by default, unless when it the currency is disabled. V5 image

V8 image

When the "choose currency" knob is used, the app breaks.

2024-08-09 17 43 48 2024-08-09 17 43 48

This is your best catch so far 😆 . I somehow must have thought that currencies is a list of currencies where you'd pick a single currency from and created a dropdown for it rather than using the currency array for configuring the input. It's fixed now, please have another look.