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.

Sign up for GitHub

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

Jump to bottom

Viewport-addon Allow setting callback to be called whenever viewport changes #3283

Merged
merged 9 commits into from
Mar 28, 2018
Merged

Viewport-addon Allow setting callback to be called whenever viewport changes #3283

merged 9 commits into from
Mar 28, 2018

Conversation

mshaaban088
Copy link
Contributor

@mshaaban088 mshaaban088 commented Mar 26, 2018 •
edited
Loading

Issue: #3223
@SimenB Please also review

What I did

Add the ability to pass onViewportChange callback to withViewport decorator, which will be called whenever viewport changes.

Signature

It has the following signature

function onViewportChange( { viewport } ) {
  // React upon changes
}

How to use

Either with global decorator (recommended), or per story

import { withViewport } from '@storybook/addon-viewport';

addDecorator(withViewport({
  onViewportChange({ viewport }) {
    console.log('Viewport changed:', viewport);
  }
}));

... and below is a screenshot from the storybook

screen shot 2018-03-28 at 00 57 44

How to test

yarn test

Is this testable with jest or storyshots?
Yes

Does this need a new example in the kitchen sink apps?
Yes and added to Viewports-addon stories

Does this need an update to the documentation?
Yes and added

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

@Hypnosphi
Copy link
Member

Please fix unit tests

@mshaaban088
Copy link
Contributor Author

@Hypnosphi thanks for reviewing!

I'm going to fix/add old/new tests probably today

@codecov
Copy link

codecov bot commented Mar 26, 2018 •
edited
Loading

Codecov Report

Merging #3283 into master will increase coverage by 0.36%.
The diff coverage is 73.97%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3283      +/-   ##
==========================================
+ Coverage   35.33%   35.69%   +0.36%     
==========================================
  Files         470      472       +2     
  Lines       10018    10104      +86     
  Branches      944      965      +21     
==========================================
+ Hits         3540     3607      +67     
- Misses       5879     5880       +1     
- Partials      599      617      +18
Impacted Files Coverage Δ
addons/viewport/src/preview/index.js 100% <ø> (+11.11%) ⬆️
addons/viewport/src/shared/index.js 100% <100%> (ø) ⬆️
addons/viewport/src/preview/components/Viewport.js 95.74% <100%> (ø)
addons/viewport/src/manager/components/Panel.js 37.1% <50%> (+1.07%) ⬆️
addons/viewport/src/preview/withViewport.js 77.27% <61.53%> (ø)
app/mithril/src/server/utils.js 0% <0%> (-100%) ⬇️
lib/ui/src/modules/ui/containers/stories_panel.js 26.31% <0%> (ø) ⬆️
addons/jest/src/components/PanelTitle.js 0% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <0%> (ø) ⬆️
... and 77 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 46c7c08...0242e1d. Read the comment docs.

@mshaaban088 mshaaban088 force-pushed the master branch 2 times, most recently from ca849dd to c505637 Compare March 27, 2018 16:51
@mshaaban088 mshaaban088 force-pushed the master branch 5 times, most recently from 3a00773 to a5f59f7 Compare March 28, 2018 00:18
@mshaaban088 mshaaban088 changed the title [WIP] viewport-addon Allow setting callback to be called whenever viewport changes Viewport-addon Allow setting callback to be called whenever viewport changes Mar 28, 2018
@mshaaban088
Copy link
Contributor Author

@Hypnosphi I've fixed old tests, added new ones, added stories, and updated documentation.

Please take a look.

P.S there is something weird with danger, could you also please check it?

@Hypnosphi
Copy link
Member

It says "PR is marked with "in progress" label." =)

@mshaaban088
Copy link
Contributor Author

facepalm

Hypnosphi
Hypnosphi reviewed Mar 28, 2018
View reviewed changes
addons/viewport/src/shared/index.js
@@ -15,55 +15,63 @@ export const INITIAL_VIEWPORTS = {
margin: '0',
boxShadow: 'none',
},
type: 'desktop',
Copy link
Member

Choose a reason for hiding this comment

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

Is this property documented anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

AFAIK Viewport definition is not documented, so it's a good idea to start doing so

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Hypnosphi
Hypnosphi requested changes Mar 28, 2018
View reviewed changes
examples/official-storybook/stories/addon-viewport.stories.js
@@ -18,10 +20,26 @@ storiesOf('Addons|Viewport.Custom Default (Kindle Fire 2)', module)
I've inherited <b>Kindle Fire 2</b> viewport from my parent.
</Panel>
))
.add('Overridden', () => (
<Viewport name="iphone6">
Copy link
Member

Choose a reason for hiding this comment

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

Does Viewport still work? If so, let's keep a story using it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it makes sense

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@Hypnosphi Hypnosphi merged commit 9621a0a into storybookjs:master Mar 28, 2018
@SimenB SimenB mentioned this pull request Mar 28, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Hypnosphi Hypnosphi Hypnosphi approved these changes

@danielduan danielduan Awaiting requested review from danielduan

@saponifi3d saponifi3d Awaiting requested review from saponifi3d saponifi3d is a code owner

@usulpro usulpro Awaiting requested review from usulpro usulpro is a code owner

Assignees
No one assigned
Labels
addon: viewport feature request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mshaaban088 @Hypnosphi