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: multiple mapped args return array of labels #22169

Merged
merged 10 commits into from
Apr 27, 2023

Conversation

gitstart
Copy link
Contributor

Issue: #22042

What I did

Fix issue of configured mapping + control.type=multi-select argType component is rendered with wrong mapped arg value.

How to test

  • Start a sandbox template: yarn task --task dev --template react-vite/default-ts
  • In sandbox/react-vite-default-ts/src/stories/Button.stories.ts
const arrows = {
  ArrowUp: { name: 'ArrowUp' },
  ArrowDown: { name: 'ArrowDown' },
  ArrowLeft: { name: 'ArrowLeft' },
  ArrowRight: { name: 'ArrowRight' },
};

const meta = {
  title: 'Example/Button',
  component: Button,
  tags: ['autodocs'],
  argTypes: {
    backgroundColor: { control: 'color' },
    arrows: {
      options: Object.keys(arrows),
      mapping: arrows,
      control: {
        type: 'multi-select',
        labels: {
          ArrowUp: 'Up',
          ArrowDown: 'Down',
          ArrowLeft: 'Left',
          ArrowRight: 'Right',
        },
      },
    },
  },
} satisfies Meta<typeof Button>;
  • Update Button.tsx with adding new prop arrows and console.log it inside Button render function
  • Verify logged prop arrows value is corrected mapped array object

Co-authored-by: phunguyenmurcul <51897872+phunguyenmurcul@users.noreply.github.com>
@gitstart gitstart marked this pull request as ready for review April 19, 2023 17:06
@gitstart
Copy link
Contributor Author

@JReinhold Could you please review this PR?

Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great find!

Just to summarize the issue for myself here. The problem with the current implementation, is that when there are multiple selected, the arg will be an array of the selected args. and so when we do val in mapping it will return false, because val will not be the actual key in the mapping, it will be a list of keys instead.

But this PR makes it so we handle it differently for arrays, and ensure that arrays of keys are mapped like a single key would be.

Would you be open to add some template stories that ensures this works? I think they would fit in here: https://github.com/storybookjs/storybook/blob/next/code/lib/store/template/stories/argTypes.stories.ts
AFAIK we don't have any stories that covers args mapping at all, so it would be great if you could add some for different use cases.

Let me know if you need more guidance on how to add and test template stories.

code/lib/preview-api/src/modules/store/csf/prepareStory.ts Outdated Show resolved Hide resolved
gitstart and others added 2 commits April 21, 2023 15:44
Co-authored-by: phunguyenmurcul <51897872+phunguyenmurcul@users.noreply.github.com>
gitstart and others added 2 commits April 24, 2023 16:37
Co-authored-by: phunguyenmurcul <51897872+phunguyenmurcul@users.noreply.github.com>
@gitstart gitstart marked this pull request as draft April 24, 2023 17:46
@gitstart gitstart marked this pull request as ready for review April 24, 2023 18:23
gitstart and others added 2 commits April 25, 2023 15:18
Co-authored-by: phunguyenmurcul <51897872+phunguyenmurcul@users.noreply.github.com>
@gitstart gitstart requested a review from JReinhold April 25, 2023 15:44
@gitstart
Copy link
Contributor Author

Would you be open to add some template stories that ensures this works?

@JReinhold I added argMapping template stories. Could you please take a look?

@JReinhold JReinhold added the ci:daily Run the CI jobs that normally run in the daily job. label Apr 27, 2023
Copy link
Contributor

@JReinhold JReinhold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is great, thanks! 💪

@JReinhold JReinhold added the patch:yes Bugfix & documentation PR that need to be picked to main branch label Apr 27, 2023
@JReinhold JReinhold merged commit 7b89042 into storybookjs:next Apr 27, 2023
tmeasday added a commit that referenced this pull request Apr 28, 2023
tmeasday added a commit that referenced this pull request Apr 28, 2023
@shilman shilman added the patch:done Patch/release PRs already cherry-picked to main/release branch label May 3, 2023
shilman pushed a commit that referenced this pull request May 3, 2023
fix: multiple mapped args return array of labels
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
args bug ci:daily Run the CI jobs that normally run in the daily job. patch:done Patch/release PRs already cherry-picked to main/release branch patch:yes Bugfix & documentation PR that need to be picked to main branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants