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 small design update to addon info package #1213

Merged
merged 3 commits into from
Jun 9, 2017

Conversation

lucasmotta
Copy link
Contributor

Issue:
When using the "info" addon inline, the current designs blends in too much with the component, making it quite confusing and sometimes difficult to preview.

screen shot 2017-06-07 at 16 45 13

What I did

This change wraps the Story info into a "box" with a light border and shadow, also adds some more padding and margin to give a bit more room.

screen shot 2017-06-07 at 16 45 01

When using the "info" addon inline, the current designs blends in
too much with the component, making it quite confusing and sometimes
difficult to preview.
This change wraps the Story info into a "box" with a light border and
shadow, also adds some more padding and margin to give a bit more room.
@ndelangen
Copy link
Member

We've been discussing the info addon quite a bit lately, and most of us agree it's contents should not be in the preview at all. We should move it into a panel.

What's your take on this?

@lucasmotta
Copy link
Contributor Author

@ndelangen after submitting this PR I've found those discussions about moving the info addon to its own panel, and it makes much more sense.

If it's something that's near from happening, then I wouldn't even bother merging it.

@ndelangen
Copy link
Member

This is super easy to merge now and release as 3.0.2. I'm OK with that actually.

Moving the contents into a panel will not be part of 3.0.2.

I'd really like everyone's input on wether we can just simply move the content of addon-info into a panel and what strategy we should have:

  1. Move it, and not provide alternatives (users can use an old version for as long as it stays compatible)
  2. Move it and provide a setting so users can choose to render it in preview
  3. Not move it, but provide a setting so it can be moved into a panel
  4. Not move it, and write a new addon that renders in a panel (fork)

Obviously (1) is preferable in the long game, for us the maintainers. But what do users want?

@codecov
Copy link

codecov bot commented Jun 8, 2017

Codecov Report

Merging #1213 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1213   +/-   ##
=======================================
  Coverage   13.72%   13.72%           
=======================================
  Files         207      207           
  Lines        4640     4640           
  Branches      527      511   -16     
=======================================
  Hits          637      637           
- Misses       3533     3564   +31     
+ Partials      470      439   -31
Impacted Files Coverage Δ
addons/info/src/components/Story.js 0% <ø> (ø) ⬆️
addons/knobs/src/components/WrapStory.js 12% <0%> (ø) ⬆️
.../ui/src/modules/ui/components/layout/dimensions.js 15.62% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-start.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/client_api.js 39.28% <0%> (ø) ⬆️
addons/info/src/components/PropVal.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/components/layout/usplit.js 38.7% <0%> (ø) ⬆️
lib/ui/src/modules/api/actions/api.js 49.42% <0%> (ø) ⬆️
addons/storyshots/src/test-bodies.js 0% <0%> (ø) ⬆️
...codemod/src/transforms/update-organisation-name.js 40.62% <0%> (ø) ⬆️
... and 18 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 85da42c...d6b3d13. Read the comment docs.

@shilman
Copy link
Member

shilman commented Jun 8, 2017

@ndelangen @lucasmotta full discussion of the panel proposal is here: #1147

@ndelangen ndelangen added this to the v3.0.2 milestone Jun 9, 2017
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