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 TableComponent option for addon-info #2400

Conversation

terrencewwong
Copy link
Contributor

Issue:

What I did

I added an option TableProp that allows you to configure how the table is rendered for addon-info. You can find more details in this issue: #2397

Here's a gif that shows how you could use this option to remove the required column and instead render an asterisk next to the prop name.
example

How to test

Not sure :/ I manually tested inside the kitchen sink.

Is this testable with jest or storyshots?
No I don't think so

Does this need a new example in the kitchen sink apps?
Yes, it's included in the PR :)

Does this need an update to the documentation?
Yes, it's included in the PR :)

If your answer is yes to any of these, please make sure to include it in your PR.

@Hypnosphi
Copy link
Member

Hypnosphi commented Nov 29, 2017

Live example for reference

@Hypnosphi Hypnosphi changed the base branch from master to release/3.3 November 29, 2017 23:13
@Hypnosphi
Copy link
Member

This looks like a valuable feature to me.

I changed the base branch for this PR to release/3.3, because that's where the active development happens now (sorry for inconvenience). Can you please resolve the conflict?

After that, you may need to run yarn test --core --update to update storyshots

@terrencewwong terrencewwong force-pushed the TableComponent_option_for_addon-info branch from d89c448 to 7b8f765 Compare December 4, 2017 12:17
@terrencewwong terrencewwong force-pushed the TableComponent_option_for_addon-info branch from 7b8f765 to 0f38aad Compare December 4, 2017 12:18
@codecov
Copy link

codecov bot commented Dec 4, 2017

Codecov Report

Merging #2400 into release/3.3 will increase coverage by <.01%.
The diff coverage is 27.39%.

Impacted file tree graph

@@               Coverage Diff               @@
##           release/3.3    #2400      +/-   ##
===============================================
+ Coverage        18.76%   18.77%   +<.01%     
===============================================
  Files              348      349       +1     
  Lines             8200     8212      +12     
  Branches           897      900       +3     
===============================================
+ Hits              1539     1542       +3     
- Misses            5978     5987       +9     
  Partials           683      683
Impacted Files Coverage Δ
addons/info/src/components/Story.js 31.1% <ø> (+0.29%) ⬆️
addons/info/src/index.js 90.9% <ø> (ø) ⬆️
addons/info/src/components/makeTableComponent.js 21.68% <26.47%> (ø)
addons/info/src/components/PropTable.js 29.78% <40%> (+4.78%) ⬆️
app/react/src/server/utils.js 0% <0%> (-53.58%) ⬇️
addons/knobs/src/components/types/Button.js 11.9% <0%> (ø) ⬆️
addons/knobs/src/components/PropForm.js 9.3% <0%> (ø) ⬆️
addons/info/src/components/types/Signature.js 16.66% <0%> (ø) ⬆️
addons/knobs/src/angular/helpers.js 0% <0%> (ø) ⬆️
...-native/src/preview/components/OnDeviceUI/style.js 0% <0%> (ø) ⬆️
... and 46 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6f52f5e...0f38aad. Read the comment docs.

Copy link
Member

@Hypnosphi Hypnosphi left a comment

Choose a reason for hiding this comment

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

LGTM

@Hypnosphi Hypnosphi merged commit f2fe1a8 into storybookjs:release/3.3 Dec 4, 2017
@danielduan
Copy link
Member

Since it's going into 3.3, why don't we just make that a new default to drop the required column in favor of the *?

I also think having monospace font for the proptype value could be useful as a default as well. Technically this isn't a breaking change since the same data is still presented, although in a slightly different way.

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

Successfully merging this pull request may close these issues.

3 participants