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

Move addon-info into addons panel #1501

Closed
wants to merge 44 commits into from
Closed

Move addon-info into addons panel #1501

wants to merge 44 commits into from

Conversation

usulpro
Copy link
Member

@usulpro usulpro commented Jul 21, 2017

Issue: #1147

What I did

image

withInfo() puts info into Info panel

decoratorInfo() keeps previous behavior

How to test

run cra-kitchen-sink

Todo

@codecov
Copy link

codecov bot commented Jul 21, 2017

Codecov Report

Merging #1501 into master will decrease coverage by 15.69%.
The diff coverage is 29.46%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1501      +/-   ##
==========================================
- Coverage   35.74%   20.04%   -15.7%     
==========================================
  Files         472      259     -213     
  Lines       10134     5887    -4247     
  Branches     1196      700     -496     
==========================================
- Hits         3622     1180    -2442     
+ Misses       5784     4157    -1627     
+ Partials      728      550     -178
Impacted Files Coverage Δ
addons/info/src/components/Node.js 38.88% <ø> (-55.32%) ⬇️
addons/info/src/components/PropTable.js 12.5% <0%> (-72.61%) ⬇️
addons/info/register.js 0% <0%> (ø)
...s/info/src/components/stories/mock-data/widget.jsx 0% <0%> (ø)
addons/info/src/panel/empty_panel.js 0% <0%> (ø)
addons/info/src/manager.js 0% <0%> (ø)
...ons/info/src/components/stories/mock-data/index.js 0% <0%> (ø)
addons/info/src/components/stories/index.js 0% <0%> (ø)
addons/info/src/panel/index.js 0% <0%> (ø)
addons/info/src/config.js 100% <100%> (ø)
... and 499 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 d0d6e4f...079bae9. Read the comment docs.

@usulpro usulpro added this to the v3.3.0 milestone Jul 21, 2017
@shilman
Copy link
Member

shilman commented Jul 21, 2017

Awesome!! So excited to see the progress on this and it will be a huge benefit for Storybook! 🎉

A few thoughts:

  1. What does Add option to switch to a specific panel #811 have to do with this? I think it's unrelated and better to keep it in a separate PR, but maybe I don't understand something.
  2. Can we please remove (or at least deprecate) the "preview decorator" support? I think it's an anti-pattern for Storybook because (1) it messes up testing, and (2) it makes the addon react-specific (unnecessarily), (3) it complicates the code to support two modes

@glambert
Copy link

glambert commented Jul 21, 2017

Quick comment on the styling of the Info panel: I wouldn't make it so "boxy", and keep the padding to the minimum to leave as much screen size as possible. Is this planned for an upcoming release?

@alterx
Copy link
Member

alterx commented Jul 21, 2017

+1 to deprecate (or, better yet, remove) inline support. It's one of the things that makes this (and potentially other) add-on difficult to use with, Angular, for example.

@ndelangen
Copy link
Member

I think this is an opportunity to move some UI into lib/components !

@usulpro usulpro changed the base branch from release/3.3 to master August 8, 2017 21:15
@usulpro
Copy link
Member Author

usulpro commented Aug 8, 2017

  • remove decorator

  • update styles to fit small screens

  • rename setDefaults to setInfoOptions

Reasons:
  1. Setting options for an addon is a good idea and we might want to include it to withX standardization so I want that name to be relevant to addon's name.

  2. This function could be used not only to initiate setting so I remove 'default' from name

  3. The use cases are very similar to setOptions

.
  • setInfoOptions, defaultOptions and withInfo should support the same set of options. setInfoOptions and withInfo has same interface related to textOrOptions

  • rename options.text to options.info

Reasons:
  1. It supports not only plain text but markdown as well so text could confuse users.

  2. It's a "leading" prop and could be passed directly without {options} so name info could highlight it as a main.

  3. Same as an inner prop of Story

counter-argument: since we publish v3.2 it will be a "breaking" change. So I don't mind to revert it to the previous name.

.

How to test

Check different options of "Addon info" from cra-kitchen-sink

@usulpro
Copy link
Member Author

usulpro commented Aug 8, 2017

Known issues at this moment:

  • Shows "No info provided..." after switching to another addon panel

  • Poor markdown support

@shilman
Copy link
Member

shilman commented Aug 9, 2017

@usulpro how about instead of options.text => options.markdown?

@usulpro
Copy link
Member Author

usulpro commented Aug 9, 2017

@shilman This name clearly reflects the type of data. But on the other hand, all other props are named in accordance with their purpose. e.g.: header, source so I'm not sure that it would fit the overall style. I guess it could be something like description from this point of view.
But of course, let's choose what is most convenient for users

And why you don't like info name? We could do the same for addon-notes: set notes as the "main" prop and use it in a "short" version.

@shilman
Copy link
Member

shilman commented Aug 10, 2017

@usulpro i have a proposal for an updated addons API and I think it becomes confusing if we have the main property have the same name as the addon:

https://gist.github.com/shilman/792dc25550daa9c2bf37238f4ef7a398#options-types

We haven't discussed this proposal, and there might be other better proposals out there. But if the proposal goes through then it would be better to call it anything other than info IMHO (markdown, summary, etc.) because info.summary makes sense but info.info is confusing. LMK what you think.

@usulpro
Copy link
Member Author

usulpro commented Aug 10, 2017

@shilman Let's take summary, it looks nice to me!

@danielduan danielduan added this to the v3.4.0 milestone Oct 31, 2017
@danielduan
Copy link
Member

Closing this because there hasn't been any activity here for a while. Feel free to reopen when this is ready for discussion again.

@danielduan danielduan closed this Nov 14, 2017
@Hypnosphi
Copy link
Member

Hypnosphi commented Nov 14, 2017

cra-kitchen-sink is more like a showcase representing the finished products

For me, it's a single point for all the reproductions. And given that we have nesting, I don't think that there's any issue with having more stories there. Especially after @danielduan has refactored each example scope into a separate file

@Hypnosphi Hypnosphi reopened this Nov 14, 2017
@ndelangen
Copy link
Member

😢

@danielduan
Copy link
Member

@Hypnosphi I agree that this should be worked on but I closed this because there is a significant amount of work left and there hasn't been any progress here since almost 3 months ago.

If you'd like to take over the PR, please do. I think you can branch off of this and create a new PR.

@Hypnosphi
Copy link
Member

I think it’s a thing that really needs to be done, but I’m not sure I’m able to do it properly. What should I do in that case?

@danielduan
Copy link
Member

danielduan commented Nov 15, 2017

I don't think there is a right person for this but @usulpro definitely has more experience working in info than anyone else. I think the difficult part is coming to a consensus as to what the future info panel will look like and how it will function, much of which has been explored here.

I think what's here is a very good start. The rest of the work looks like it's fixing merge conflicts and integrating the changes with ongoing development since the last commit here. Migrating the Storybook over shouldn't be too difficult. We can then discuss if the changes here are appropriate.

Personally, I think the process of documenting a component could be even more automated such that we perhaps don't need to manually create entries for excluding and including certain components, etc and generating more documentation automatically. That's a separate discussion that can come after this major refactor.

@Hypnosphi Hypnosphi changed the base branch from release/3.3 to master December 23, 2017 23:54
@shilman shilman modified the milestones: v3.4.0, v4.0.0 Feb 3, 2018
@stale
Copy link

stale bot commented Mar 20, 2018

Hi everyone! Seems like there hasn't been much going on in this issue lately. If there are still questions, comments, or bugs, please feel free to continue the discussion. Unfortunately, we don't have time to get to every issue. We are always open to contributions so please send us a pull request if you would like to help. Inactive issues will be closed after 60 days. Thanks!

@igor-dv
Copy link
Member

igor-dv commented Jun 14, 2018

Does anyone want to finish it for the v4 release?

@igor-dv
Copy link
Member

igor-dv commented Aug 28, 2018

I think we don't need to hold this one open. Let's revisit the implementation/rationale in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addon: info bug cleanup Minor cleanup style change that won't show up in release changelog feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants