Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix runtime error in Slider storybook #2488

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

nayounsang
Copy link
Contributor

@nayounsang nayounsang commented Nov 7, 2024

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

Copy link

changeset-bot bot commented Nov 7, 2024

⚠️ No Changeset found

Latest commit: 8c755e5

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

Copy link

channeltalk bot commented Nov 7, 2024

Copy link

codecov bot commented Nov 7, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.27%. Comparing base (1afe04b) to head (8c755e5).
Report is 7 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      913      904    -9     
=======================================
  Hits         2451     2451           
- Misses        497      523   +26     
+ Partials       31        5   -26     

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

Copy link
Contributor

github-actions bot commented Nov 7, 2024

Chromatic Report

🚀 Congratulations! Your build was successful!

@yangwooseong
Copy link
Collaborator

yangwooseong commented Nov 8, 2024

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

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

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

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

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

@yangwooseong yangwooseong added the fix PR related to bug fix label Nov 8, 2024
@nayounsang nayounsang changed the title fix: process value props to number[] or undefined Fix runtime error in Slider storybook Nov 8, 2024
@nayounsang
Copy link
Contributor Author

nayounsang commented Nov 8, 2024

@yangwooseong

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

하지만 고민이 있어요.
공통되는 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를 설정 가능한 모습)
image
image

@yangwooseong
Copy link
Collaborator

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

  1. 스크롤이 생기는 것은 상관 없다고 생각해요. 다만 Story 작성할 떄 포맷을 다른 곳과 맞춰주시면 좋겠습니다. CSF3 과 CSF2의 차이를 확인해주세요. (#)
export const Primary = {
  args: {
    value: [5],
  },
} satisfies StoryObj<typeof meta>

export const Uncontrolled = {
  args: {
    defaultValue: [3, 7],
  },
} satisfies StoryObj<typeof meta>
  1. controlled 일 때는 defaultValue 를 숨기고, uncontrolled 일 때는 value 를 숨기는 것도 가능합니다(#). 보통은 안숨겨도 무방하겠지만 에러가 나서 숨기는 게 깔끔할 것 같습니다.
const meta: Meta<typeof Slider> = {
  component: Slider,
  argTypes: {
    ... // 생략 
    value: {
      if: {
        exists: true,
        arg: 'value',
      },
    },
    defaultValue: {
      if: {
        exists: true,
        arg: 'defaultValue',
      },
    },
  },
}
  1. 컴포넌트 버그처럼 라이브러리 외부로 노출되는 버그가 아니기 때문에 changelog 에 포함되지 않아도 될것 같습니다. 그래서 changeset 은 지워주셔도 됩니다!

@nayounsang
Copy link
Contributor Author

@yangwooseong 피드백 감사합니다.
2번같은 경우엔 질문이 있어요. 현재 meta에 선언한 공통되는 arg와 argType엔 valuedefaultValue가 존재하지 않고
valuedefaultValue는 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
Copy link
Collaborator

@yangwooseong 피드백 감사합니다.

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

valuedefaultValue는 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
Copy link
Contributor Author

@yangwooseong 피드백 감사합니다.
2번같은 경우엔 질문이 있어요. 현재 meta에 선언한 공통되는 arg와 argType엔 valuedefaultValue가 존재하지 않고
valuedefaultValue는 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에 대한 컨트롤을 숨겼던 것 같은데 저런 방법이 있었군요 배워갑니다👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fix PR related to bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

value option of Slider component does not work in storybook
2 participants