Fix runtime error in Slider storybook #2488

nayounsang · 2024-11-07T09:22:19Z

Self Checklist

I wrote a PR title in English and added an appropriate label to the PR.
I wrote the commit message in English and to follow the Conventional Commits specification.
I added the changeset about the changes that needed to be released. (or didn't have to)
I wrote or updated documentation related to the changes. (or didn't have to)
I wrote or updated tests related to the changes. (or didn't have to)
I tested the changes in various browsers. (or didn't have to)
- Windows: Chrome, Edge, (Optional) Firefox
- macOS: Chrome, Edge, Safari, (Optional) Firefox

Related Issue

close #2312

Summary

I processed value props to prevent type error.

Details

First commit

I don't like it, but using a Template was the solution I thought of.
The control that inputs the array in storybook requires object input.
Maybe, I can create a custom control using a decorator. I'm not sure because I haven't tried it yet. We also need to discuss how to use this to receive input.

const Template: StoryFn<SliderProps> = (args) => {
  const processedArgs = getProcessedArgs(args)
  return <Slider {...processedArgs} />
}

export const Primary = Template.bind({})
Primary.args = {
  width: 285,
  defaultValue: [5],
  value: undefined,
  disabled: false,
  guide: [5],
  min: 0,
  max: 10,
  step: 1,
  disableTooltip: false,
}

Set existing args and manipulate necessary props in the template.
I'd like to be able to manipulate specific props into the desired format in argTypes, but I don't know how. I couldn't find this in the docs.

const getProcessedArgs = (args: SliderProps): SliderProps => {
  return { ...args, value: getProcessedValue(args.value) }
}

const getProcessedValue = (
  value: undefined | Record<number, number> | number[]
) => {
  if (value === undefined) {
    return undefined
  }
  if (Array.isArray(value) && value.every((item) => typeof item === 'number')) {
    return value
  }
  if (typeof value === 'object' && value !== null) {
    return Object.values(value)
  }
  return value
}

The function was separated into two to prepare for props that require additional processing in the future.
If JSON is number[] or undefined, this func returns original value.
If JSON is object, this func returns value of object.
In other cases, it returns as is and not reflect in the storybook.
There will be stricter guards and various cases, but I think this will need to be discussed because the code structure will be dirty (and I think it still is to some extent 🤣).
- For example, there is logic that verifies whether an object's key is a consecutive number starting from 0.

Split stories into primary(controlled) & uncontrolled.
In controlled, user can set value not 'defaultValue'.
In uncontrolled, user can set 'defaultValue' not 'value'.
Set common args in meta of Slider such as width, min, max ...
Show args conditionally in production env using argTypes.if.

Breaking change? (Yes/No)

No

References

changeset-bot · 2024-11-07T09:22:23Z

⚠️ No Changeset found

Latest commit: d2d545a

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

channeltalk · 2024-11-07T09:22:24Z

https://desk.channel.io/#/channels/1/team_chats/groups/1237/672c86cfe898413a6268

codecov · 2024-11-07T09:24:40Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.27%. Comparing base (63fbd3c) to head (d2d545a).
Report is 1 commits behind head on main.

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #2488   +/-   ##
=======================================
  Coverage   82.27%   82.27%           
=======================================
  Files         143      143           
  Lines        2979     2979           
  Branches      917      913    -4     
=======================================
  Hits         2451     2451           
  Misses        497      497           
  Partials       31       31

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

github-actions · 2024-11-07T09:24:56Z

Chromatic Report

🚀 Congratulations! Your build was successful!

yangwooseong · 2024-11-08T04:23:41Z

아마 value 에 undefined가 초기값으로 되서 object 타입으로 잡히는 게 �에러가 뜨는 원인 것 같습니다.

아예 controlled와 uncontrolled 스토리를 나누는 것은 어떨까요? Switch 의 예시를 참고하시면 될 것 같습니다.

controlled -> value의 초기값을 주고 defaultValue 을 args로 제공하지 않고
uncontrolled -> defaultValue의 초기값을 주고 value 를 args 로 제공하지 않는다

이렇게 하면 value를 sanitize 할 필요도 없어질 것 같아요!

어떤 문제를 해결하셨는지 들어나도록 PR 제목을 바꿔주세요. Fix runtime error in Slider storybook 정도면 어떨까요?

nayounsang · 2024-11-08T07:24:25Z

@yangwooseong

매우 좋은 예시네요! 두개의 스토리로 분할했어요.
수정했습니다.

하지만 고민이 있어요.
공통되는 props들의 값을 meta에서 정의했어요.

const meta: Meta<typeof Slider> = {
  component: Slider,
  argTypes: {
    minStepsBetweenThumbs: {
      control: {
        type: 'number',
      },
    },
    onValueChange: {
      action: 'onValueChange',
    },
    onValueCommit: {
      action: 'onValueCommit',
    },
  },
  args:{
    width: 285,
    disabled: false,
    guide: [5],
    min: 0,
    max: 10,
    step: 1,
    disableTooltip: false,
  }
}

그 뒤 스토리마다 변화하는 control이 있으면

export const Uncontrolled = Template.bind({})
Uncontrolled.args = {
  defaultValue: [5],
}

추가적으로 arg를 설정해주는 방식이에요.
하지만 이런 경우 meta에서 선언한 arg가 제일 먼저오고 스토리마다 설정한 arg인 value 또는 defaultValue들이 후순위로 와서 제 디바이스에선 value와 defaultValue의 경우 스크롤을 내려야 설정할 수 있었습니다.
이 방법도 괜찮은지,
아니면 const baseArgs:Partial<SliderProps> = {} 란 별도의 객체에 두 스토리 모두 공통되는 props를 설정하고 복사할지,
아니면 각 스토리마다 필요한 모든 arg들을 설정하는게 좋을지 고민이네요.

참고사진(스크롤을 내려야 value를 설정 가능한 모습)

yangwooseong · 2024-11-08T08:34:21Z

하지만 이런 경우 meta에서 선언한 arg가 제일 먼저오고 스토리마다 설정한 arg인 value 또는 defaultValue들이 후순위로 와서 제 디바이스에선 value와 defaultValue의 경우 스크롤을 내려야 설정할 수 있었습니다.

스크롤이 생기는 것은 상관 없다고 생각해요. 다만 Story 작성할 떄 포맷을 다른 곳과 맞춰주시면 좋겠습니다. CSF3 과 CSF2의 차이를 확인해주세요. (#)

export const Primary = {
  args: {
    value: [5],
  },
} satisfies StoryObj<typeof meta>

export const Uncontrolled = {
  args: {
    defaultValue: [3, 7],
  },
} satisfies StoryObj<typeof meta>

controlled 일 때는 defaultValue 를 숨기고, uncontrolled 일 때는 value 를 숨기는 것도 가능합니다(#). 보통은 안숨겨도 무방하겠지만 에러가 나서 숨기는 게 깔끔할 것 같습니다.

const meta: Meta<typeof Slider> = {
  component: Slider,
  argTypes: {
    ... // 생략 
    value: {
      if: {
        exists: true,
        arg: 'value',
      },
    },
    defaultValue: {
      if: {
        exists: true,
        arg: 'defaultValue',
      },
    },
  },
}

컴포넌트 버그처럼 라이브러리 외부로 노출되는 버그가 아니기 때문에 changelog 에 포함되지 않아도 될것 같습니다. 그래서 changeset 은 지워주셔도 됩니다!

nayounsang · 2024-11-08T09:03:29Z

@yangwooseong 피드백 감사합니다.
2번같은 경우엔 질문이 있어요. 현재 meta에 선언한 공통되는 arg와 argType엔 value와 defaultValue가 존재하지 않고
value와 defaultValue는 Primary와 Uncontrolled 각각의 스토리에서 정의하고 있어요.
선언을 하지 않은 경우 스토리북이 숨기는 걸로 알고 있어서요. 예를들어 Uncontorlled는 arg와 argType에 value가 없으므로 스토리북이 숨기고 제 디바이스에서도 현재 그렇게 보여요.
제가 알기로 if는

    parent: { control: 'select', options: ['one', 'two', 'three'] },
 
    // 👇 Only shown when `parent` arg exists
    parentExists: { if: { arg: 'parent', exists: true } },

이 예와 같이 어떤 arg에 상태에 따라 다른 arg를 조건부로 랜더링하는 걸로 알고 있어서 질문드립니다

yangwooseong · 2024-11-08T09:44:41Z

@yangwooseong 피드백 감사합니다.

2번같은 경우엔 질문이 있어요. 현재 meta에 선언한 공통되는 arg와 argType엔 value와 defaultValue가 존재하지 않고

value와 defaultValue는 Primary와 Uncontrolled 각각의 스토리에서 정의하고 있어요.

선언을 하지 않은 경우 스토리북이 숨기는 걸로 알고 있어서요. 예를들어 Uncontorlled는 arg와 argType에 value가 없으므로 스토리북이 숨기고 제 디바이스에서도 현재 그렇게 보여요.

제가 알기로 if는
    parent: { control: 'select', options: ['one', 'two', 'three'] },

 

    // 👇 Only shown when `parent` arg exists

    parentExists: { if: { arg: 'parent', exists: true } },
이 예와 같이 어떤 arg에 상태에 따라 다른 arg를 조건부로 랜더링하는 걸로 알고 있어서 질문드립니다

저도 개발모드에서는 숨겨지는데 배포된 것을 보면 안숨겨지고 그대로 있어서요. 위에 댓글에 있는 스토리북 프리뷰를 보시면 확인가능합니다
#2488 (comment)

로컬에서 배포해서 테스트 하려면 yarn workspace @channel.io/bezier-react storybook-build 명령을 활용하시면 됩니다

nayounsang · 2024-11-08T13:57:08Z

@yangwooseong 피드백 감사합니다.
2번같은 경우엔 질문이 있어요. 현재 meta에 선언한 공통되는 arg와 argType엔 value와 defaultValue가 존재하지 않고
value와 defaultValue는 Primary와 Uncontrolled 각각의 스토리에서 정의하고 있어요.
선언을 하지 않은 경우 스토리북이 숨기는 걸로 알고 있어서요. 예를들어 Uncontorlled는 arg와 argType에 value가 없으므로 스토리북이 숨기고 제 디바이스에서도 현재 그렇게 보여요.
제가 알기로 if는
    parent: { control: 'select', options: ['one', 'two', 'three'] },

 

    // 👇 Only shown when `parent` arg exists

    parentExists: { if: { arg: 'parent', exists: true } },
이 예와 같이 어떤 arg에 상태에 따라 다른 arg를 조건부로 랜더링하는 걸로 알고 있어서 질문드립니다
저도 개발모드에서는 숨겨지는데 배포된 것을 보면 안숨겨지고 그대로 있어서요. 위에 댓글에 있는 스토리북 프리뷰를 보시면 확인가능합니다 #2488 (comment)

로컬에서 배포해서 테스트 하려면 yarn workspace @channel.io/bezier-react storybook-build 명령을 활용하시면 됩니다

아! 배포환경을 생각 못했네요😅 fix한 커밋 로컬에서 테스트했고 잘 숨겨집니다!
지금 기억난건데 control: false 로 특정 arg에 대한 컨트롤을 숨겼던 것 같은데 저런 방법이 있었군요 배워갑니다👍

sungik-choi · 2024-11-12T02:36:57Z

packages/bezier-react/src/components/Slider/Slider.stories.tsx

+    value: {
+      if: {
+        exists: true,
+        arg: 'value',
+      },
+    },
+    defaultValue: {
+      if: {
+        exists: true,
+        arg: 'defaultValue',
+      },
+    },


nayounsang requested review from sungik-choi and yangwooseong as code owners November 7, 2024 09:22

yangwooseong assigned nayounsang Nov 8, 2024

yangwooseong added the fix PR related to bug fix label Nov 8, 2024

nayounsang changed the title ~~fix: process value props to number[] or undefined~~ Fix runtime error in Slider storybook Nov 8, 2024

yangwooseong approved these changes Nov 11, 2024

View reviewed changes

sungik-choi approved these changes Nov 12, 2024

View reviewed changes

nayounsang added 6 commits November 12, 2024 13:33

fix: process value props to number[] or undefined

e41a882

chore: add patch changeset

7054c2c

refactor: split slider story into control and uncontrol

9cc10c7

chore: delete change log

90e2dc1

refactor: migrate format from csf2 to csf3

abd9d2c

fix: show args conditionally in pord

d2d545a

yangwooseong force-pushed the slider-storybook-value branch from 8c755e5 to d2d545a Compare November 12, 2024 04:33

yangwooseong merged commit d3d0acf into channel-io:main Nov 12, 2024
9 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix runtime error in Slider storybook #2488

Fix runtime error in Slider storybook #2488

nayounsang commented Nov 7, 2024 •

edited

Loading

changeset-bot bot commented Nov 7, 2024 •

edited

Loading

channeltalk bot commented Nov 7, 2024

codecov bot commented Nov 7, 2024 •

edited

Loading

github-actions bot commented Nov 7, 2024 •

edited

Loading

yangwooseong commented Nov 8, 2024 •

edited

Loading

nayounsang commented Nov 8, 2024 •

edited

Loading

yangwooseong commented Nov 8, 2024

nayounsang commented Nov 8, 2024

yangwooseong commented Nov 8, 2024

nayounsang commented Nov 8, 2024

sungik-choi Nov 12, 2024

Fix runtime error in Slider storybook #2488

Fix runtime error in Slider storybook #2488

Conversation

nayounsang commented Nov 7, 2024 • edited Loading

Self Checklist

Related Issue

Summary

Details

Breaking change? (Yes/No)

References

changeset-bot bot commented Nov 7, 2024 • edited Loading

⚠️ No Changeset found

channeltalk bot commented Nov 7, 2024

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

github-actions bot commented Nov 7, 2024 • edited Loading

Chromatic Report

yangwooseong commented Nov 8, 2024 • edited Loading

nayounsang commented Nov 8, 2024 • edited Loading

yangwooseong commented Nov 8, 2024

nayounsang commented Nov 8, 2024

yangwooseong commented Nov 8, 2024

nayounsang commented Nov 8, 2024

sungik-choi Nov 12, 2024

Choose a reason for hiding this comment

nayounsang commented Nov 7, 2024 •

edited

Loading

changeset-bot bot commented Nov 7, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading

github-actions bot commented Nov 7, 2024 •

edited

Loading

yangwooseong commented Nov 8, 2024 •

edited

Loading

nayounsang commented Nov 8, 2024 •

edited

Loading