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

Add filtering prop tables by excluding or including #163

Merged
merged 8 commits into from
May 6, 2019

Conversation

lonyele
Copy link
Collaborator

@lonyele lonyele commented Apr 29, 2019

closes Issue: #159

Hi, first I'm really sorry for being late on this new feature. I've been pretty ok before I said I would take this feature, but as soon as I take it I've got a lot of tasks from my work(magically) I'll try to be as responsive as possible, but I predict my heavy workload goes until next 1 or 2 weeks,

Basically, I've implemented as we talked at #159. I chose putting actual React components instead of string of React component names because something like

import WhateverNameFromDefault from "./DefaultButton";
import { ExplicitButton as AnotherButton} from "./ExplicitButton";

would not work with string of names. However if actual React component is used, its displayName and name can be tracked to its real value.

Please take a look and give me some feedbacks. Aside from code reviews, I think more can be added after the main logic is acceptable(adding more examples, Docs, examples on Vue etc...)

@tuchk4
Copy link
Owner

tuchk4 commented May 6, 2019

LGFM

Let's merge it?

@lonyele
Copy link
Collaborator Author

lonyele commented May 6, 2019

LGFM

Let's merge it?

That sounds good. I will add more docs and examples little bit later

@lonyele lonyele merged commit f2ee03c into tuchk4:master May 6, 2019
@lonyele lonyele changed the title [WIP] Add filtering prop tables by excluding or including Add filtering prop tables by excluding or including May 6, 2019
@tuchk4
Copy link
Owner

tuchk4 commented May 6, 2019

@lonyele released at 5.0.4-alpha.2+6bd5f8d

@archansel
Copy link

archansel commented May 13, 2019

I tried using both includePropTables and excludePropTables (in my project), but it seems includePropTables being ignore. But if I use example-react in master branch, the one that being ignored is excludePropTables @lonyele @tuchk4

Correctly included

Not excluded

storybook-readme: 5.0.4-alpha.2

@lonyele
Copy link
Collaborator Author

lonyele commented May 14, 2019

@archansel hm... what you are saying is that example-react at master branch is working fine but it is not working at your project? Can you give me more information?

  1. At your project, can you see any of console.warn(...) messages such as storybook-readme: excludePropTables is not an array. It must be an ' ...
  2. If you can't see any of these storybook-readme warning messages, I found that storybook itself can be sometimes flaky, so would you restart storybook again and see the same thing happens again?

If none of the above works, then can you show me the code where you use excludePropTables and includePropTables and the story where you used these feature.

@archansel
Copy link

Both (master branch and my project) gave incorrect behavior

Master Branch
Steps:

  1. Clone repo
  2. npm install && npm run storybook in example-react directory
  3. Goto PropsTable stories, the Exclude globally story shows INCORRECT behavior. Even the readme says You can notice from below that the component is showing but propTable is not showing eventhough <!-- PROPS --> is specified., the props table still showing up

I don't know whether Include locally gave a correct result or not because the props table not excluded globally

My Project
This is my story

import React, { useState } from 'react';
import { storiesOf } from '@storybook/react';

import { CheckboxInput } from '../index';
import CheckboxInputReadme from './CheckboxInput.readme.md';

/* eslint-disable react/prop-types */
const value = [{ value: 'option-2', label: 'My Option #2' }];
const options = [{ value: 'option-1', label: 'My Option #1' }, { value: 'option-2', label: 'My Option #2' }, { value: 'option-3', label: 'My Option #3' }];
const StoryComponent = ({ value: propValue, onChange: propOnChange, ...props }) => {
  const [currentValue, setCurrentValue] = useState(propValue);
  const onChange = values => {
    setCurrentValue(values);
    if (propOnChange) propOnChange(values);
  };
  return <CheckboxInput value={currentValue} onChange={onChange} {...props} />;
};

storiesOf('Forms|Input.CheckboxInput', module)
  .addParameters({
    readme: {
      sidebar: CheckboxInputReadme,
    },
  })
  .add('Default', () => <StoryComponent name="my-options" value={value} options={options} />)
  .add('With Disabled State', () => <CheckboxInput name="my-options" value={value} options={options} disabled />);

It has two stories Default and With Disabled State, my goal is to include CheckboxInput props table in all of my stories for the current context Forms|Input.CheckboxInput. So I added includePropTables in addParameters

.addParameters({
    readme: {
      sidebar: CheckboxInputReadme,
      includePropTables: [CheckboxInput],
    },
  })

Originally, the Default story doesn't show any props table and the With Disabled State story shows CheckboxInput props table. Now, even using includePropTables: [CheckboxInput] option, the Default story still doesn't have props table from CheckboxInput. In the other hand, if I put excludePropTables: [CheckboxInput] in option, both Default and With Disabled State don't have CheckboxInput props table. Am I doing it wrong?

So, as you can see (I hope its clearer now), in master branch, the incorrect behavior is excludePropTables but in my project the incorrect behavior is includePropTables

PS: No warning in the console

@lonyele
Copy link
Collaborator Author

lonyele commented May 15, 2019

@archansel hm... first I tested this official repo's exmple-react on windows 10(chrome, firefox) and ubuntu 18.04(chrome, firefox) both cloning this repo and make up fresh CRA project with storybook-readme setting. From my laptop, everything works fine(I can't reproduce your case)

@tuchk4 would you try reproduce this behaviour? or I'd like to see if anyone seeing this problem too.(yes! you... now reading this comment)

My guesses are that

Originally, the Default story doesn't show any props table

-> This part is the problem of general propTables feature, not filteringPropTables feature. Though, I think I can investigate and make it better.

Now, even using includePropTables: [CheckboxInput] option, the Default story still doesn't have props table from CheckboxInput

-> This part is the same problem as above. From the beginning it is not showing the propTables, thus with includePropTables, it is still not showing anything.

In the other hand, if I put excludePropTables: [CheckboxInput] in option, both Default and With Disabled State don't have CheckboxInput props table. Am I doing it wrong?

-> This is a correct behaviour. Default's propTable is not showing because of above reason(propTables problem) With Disabled State's propTable is not showing because of excludePropTables

I have made some examples here(also covers your case) https://github.com/lonyele/storybook-readme-filtering-proptables-example
you can just

  1. git clone https://github.com/lonyele/storybook-readme-filtering-proptables-example
  2. yarn or npm install
  3. yarn storybook or npm run storybook
    I would investigate the propTables feature(I have some guess) while can you check my repo's example and see if it is working or not.

btw I may not be that responsive, it's still a weekday, so I don't have much time

@archansel
Copy link

I know the problem here, master by "default" still using version 5.0.3, after updating the version to 5.0.4-alpha.2, the example works perfectlly

"storybook-readme": "^5.0.3",

I think I got a better understanding of this feature. I thought includePropTables add ADDITIONAL props table (as the name implied) even if the component not in the story itself. It seems just an example without the docs still make some people (at least me) got confused 😄

This part is the problem of general propTables feature

as for general propTables issue, is there any specific issue that I can follow to get the update?

@lonyele
Copy link
Collaborator Author

lonyele commented May 16, 2019

@archansel hm... maybe... It was working fine for me. I maybe wrong but other examples(example-react etc) on this repo is linked directly to storybook-readme folder using lerna/workspace so always with the latest version I guess.

Anyway, If it's not a bug then It sounds good to me. I should probably finish additional things(more examples, docs) this weekend

About the propTables issue, I don't even know If it is a bug since current behaviour can be an intended outcome. However, I'll also investigate that on this weekend.

sorry for the confusion and I'll make explanation and usage more clear soon

@archansel
Copy link

It's a wrap then 😄

Nice additional feature, by the way. Looking forward to the stable release

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

Successfully merging this pull request may close these issues.

3 participants