viewport-addon: Make the addon configurable #3099

mshaaban088 · 2018-02-25T23:35:34Z

Note: This is my first open-source PR.

Edit

I addressed #2395 and the respective comment

Issue: Currently you cannot customize or add new viewports/devices as described in the README file

What I did

Exported a configure function to easily customize the addon
- Add new device/viewport
- Replace the entire viewports set
- Change the default viewport for all stories
Exported a decorator function and wrapper component to change the default viewport per story
Replaced the default option with a new viewport called responsive to treat the default as 1st-class citizen
Added 2 new devices, and different stories for theviewport addon to the official-storybook example
Organized the code respectively

Add new device

// config.js
import { configure, INITIAL_VIEWPORTS } from '@storybook/addon-viewport';

const extraViewports = {
  kindleFire2: {
    name: 'Kindle Fire 2',
    styles: {
      width: '600px',
      height: '963px'
    }
  },
  kindleFireHD: {
    name: 'Kindle Fire HD',
    styles: {
      width: '533px',
      height: '801px'
    }
  }
};

configure({
  viewports: {
    ...INITIAL_VIEWPORTS,
    ...extraViewports
  }
});

Use new list of devices

// config.js

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

const newViewports = {
  kindleFire2: {
    name: 'Kindle Fire 2',
    styles: {
      width: '600px',
      height: '963px'
    }
  },
  kindleFireHD: {
    name: 'Kindle Fire HD',
    styles: {
      width: '533px',
      height: '801px'
    }
  }
};

configure({
  viewports: newViewports
});

Change the default viewport

// config.js

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

configure({
  defaultViewport: 'iphone6'
});

Change the default viewport per story

// file.stories.js

import React from 'react';
import { storiesOf } from '@storybook/react';
import { withViewport, Viewport } from '@storybook/addon-viewport';

storiesOf('Kindle Fire 2', module)
  .addDecorator(withViewport('kindleFire2'))
  .add('Inherited', () => (
    <div>
      I will be displayed on <b>Kindle Fire 2</b> viewport by default.
    </div>
  ))
  .add('Overridden', () => (
    <Viewport name="iphone6">
      <div>
        I will be displayed on <b>iPhone 6</b> viewport by default.
      </div>
    </Viewport>
  ));

How to test

cd examples/official-storybook
yarn storybook
You will see 2 new devices were added to the viewport panel
Change the extra-viewports.json file
Observe the changes

Is this testable with jest or storyshots?
Yes

Does this need a new example in the kitchen sink apps?
No, I added it to the official-storybook already

Does this need an update to the documentation?
Yes

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

igor-dv

Congrats on your first PR! Great work.

igor-dv · 2018-02-26T08:05:08Z

examples/cra-kitchen-sink/.storybook/config.js


 // deprecated usage of infoAddon
 import infoAddon from '@storybook/addon-info';

+import viewports from './viewports.json';


I think that you need to move the example to the official-storybook. @Hypnosphi ?

Yes, that would be right

@igor-dv @Hypnosphi I moved it to official-storybook

igor-dv · 2018-02-26T08:06:21Z

addons/viewport/src/preview/index.js

+import addons from '@storybook/addons';
+import { ADD_VIEWPORTS_EVENT_ID, SET_VIEWPORTS_EVENT_ID } from '../shared';
+
+export function init() {}


What is this for?

I couldn't find a clear API documentation describing the preview API's, so I decided to follow the same approach as manager

Got it, but this method is a bit misleading, so let's remove it.

I removed it

codecov · 2018-02-26T16:31:53Z

Codecov Report

Merging #3099 into master will decrease coverage by 0.01%.
The diff coverage is 48.8%.

@@            Coverage Diff             @@
##           master    #3099      +/-   ##
==========================================
- Coverage    35.9%   35.89%   -0.02%     
==========================================
  Files         437      440       +3     
  Lines        9485     9620     +135     
  Branches      912      906       -6     
==========================================
+ Hits         3406     3453      +47     
- Misses       5500     5572      +72     
- Partials      579      595      +16

Impacted Files	Coverage Δ
addons/viewport/src/manager/index.js	`83.33% <ø> (ø)`
addons/viewport/src/manager/components/styles.js	`42.85% <ø> (ø)`
.../viewport/src/manager/components/RotateViewport.js	`22.72% <ø> (ø)`
addons/viewport/preview.js	`100% <100%> (ø)`
addons/viewport/src/shared/index.js	`100% <100%> (ø)`
.../viewport/src/manager/components/SelectViewport.js	`18.18% <33.33%> (ø)`
addons/viewport/src/manager/components/Panel.js	`36.02% <41.17%> (ø)`
...ns/viewport/src/manager/components/viewportInfo.js	`33.33% <45%> (ø)`
addons/viewport/src/preview/index.js	`88.88% <88.88%> (ø)`
app/vue/src/server/utils.js	`0% <0%> (-100%)`	⬇️
... and 78 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 c3d9a41...af02d9a. Read the comment docs.

mshaaban088 · 2018-02-26T16:44:53Z

@igor-dv @Hypnosphi thanks for reviewing it

Hypnosphi · 2018-02-26T18:57:28Z

addons/viewport/src/manager/components/Panel.js

+
+    this.setState({
+      viewports: {
+        ...initialViewports,


Should it be current viewports instead?

My intention was to make add works by adding new viewports to the initial ones, but indeed your point makes more sense

Hypnosphi

Please update addon documentation

mshaaban088 · 2018-02-26T20:02:24Z

@Hypnosphi I updated the documentation and addressed your comment regarding using previous viewports instead of initialViewports

igor-dv · 2018-02-27T08:01:20Z

@shilman, are we still merging the features? You wanted to lock in favor of the release.

Hypnosphi · 2018-02-27T11:58:43Z

@shilman if we're feature-frozen, we probably should edit those lines on master and add 'feature request' there: https://github.com/storybooks/storybook/blob/master/dangerfile.js#L13-L15

mshaaban088 · 2018-02-27T20:41:04Z

I'm going to rebase with master again to not have those Merge branch master into master

Hypnosphi · 2018-02-27T22:15:55Z

to not have those Merge branch master into master

We're actually OK with those

mshaaban088 · 2018-03-11T10:29:25Z

@igor-dv @Hypnosphi @danielduan @ndelangen Could you please re-review it, apart from the conflicts, do you think this way of implementing it is better?
I really appreciate your time reviewing the initial one, but wanted to make everybody happy

Hypnosphi

LGTM

Hypnosphi · 2018-03-11T18:32:57Z

Please fix eslint warnings and unit tests

mshaaban088 · 2018-03-12T00:32:08Z

@Hypnosphi Are you aware of any historical reason behind making viewports an object instead of array?
CC: @saponifi3d

Hypnosphi · 2018-03-12T11:28:10Z

No I'm not. I think we're free to change it in this PR as it wasn't exposed to user before. On the other side, it may be convenient as it is now, if user wants to pick only particular viewports from defaults or override some values in a particular viewport:

const { iphone6, ipad } = INITIAL_VIEWPORTS;

configure({
  viewports: {
    iphone6,
    ipad,
    ...newViewports
  }
});

configure({
  viewports: {
    ...INITIAL_VIEWPORTS,
    ipad: {
      ...INITIAL_VIEWPORTS.ipad,
      ...styles: {
        ...INITIAL_VIEWPORTS.ipad.styles,
        height: '1000px'
      }
    },
    ...newViewports
  }
});

mshaaban088 · 2018-03-17T14:27:02Z

@Hypnosphi This is absolutely a valid use-case.

The reason why this question is popped up, is that the order of Object.keys is not guaranteed, hence, we cannot say we will pick the first one as default which makes the tests flickering as well.

Hypnosphi · 2018-03-19T23:29:47Z

@mshaaban088 can you please resolve the conflict?

mshaaban088 · 2018-03-19T23:49:34Z

@Hypnosphi sure

- By default, viewport addon is reading both initial viewports' set and default viewport from `INITIAL_VIEWPORTS` and `DEFAULT_VIEWPORT` respectively which are exported by the addon - `configure` function is exported which accepts an object with `defaultViewport` and `viewports` - Change the default viewport by setting `defaultViewport` property to the key of the viewport (e.g. `configure({ defaultViewport: 'iphone5' })`) - Replace the entire viewports by setting brand new viewports to `viewports` property (e.g. `configure({ viewports: { brandNewDevice: { ... } } })`) - Add custom viewports by first merging both the `INITIAL_VIEWPORTS` and your custom viewports, and then pass the result as `viewports` to configure function

Fix linting issues

mshaaban088 · 2018-03-19T23:54:23Z

@Hypnosphi done

igor-dv added feature request addon: viewport labels Feb 26, 2018

igor-dv self-requested a review February 26, 2018 07:57

igor-dv reviewed Feb 26, 2018

View reviewed changes

mshaaban088 force-pushed the master branch from 378525b to ed3d9d7 Compare February 26, 2018 16:31

mshaaban088 force-pushed the master branch from ed3d9d7 to 00d4252 Compare February 26, 2018 16:37

Hypnosphi reviewed Feb 26, 2018

View reviewed changes

Hypnosphi requested changes Feb 26, 2018

View reviewed changes

mshaaban088 force-pushed the master branch from 00d4252 to 09ed6c4 Compare February 26, 2018 19:59

Hypnosphi approved these changes Feb 26, 2018

View reviewed changes

danielduan approved these changes Feb 27, 2018

View reviewed changes

mshaaban088 force-pushed the master branch from a30488b to 6f316f8 Compare February 27, 2018 20:50

Hypnosphi added the ready label Feb 28, 2018

igor-dv mentioned this pull request Feb 28, 2018

How to set addon-viewport viewport default value #2395

Closed

Hypnosphi added this to the v4.0.0 milestone Mar 8, 2018

ndelangen approved these changes Mar 8, 2018

View reviewed changes

mshaaban088 force-pushed the master branch 4 times, most recently from 915aca0 to e2ed8ca Compare March 9, 2018 14:31

mshaaban088 changed the title ~~viewport-addon: Add support for adding new viewports/devices~~ viewport-addon: Allow configuring the addon Mar 9, 2018

mshaaban088 force-pushed the master branch from e2ed8ca to f212451 Compare March 9, 2018 15:14

Hypnosphi approved these changes Mar 11, 2018

View reviewed changes

mshaaban088 force-pushed the master branch 4 times, most recently from 7e8b611 to 63d11ce Compare March 12, 2018 00:15

mshaaban088 changed the title ~~viewport-addon: Allow configuring the addon~~ viewport-addon: Make the addon configurable Mar 12, 2018

mshaaban088 mentioned this pull request Mar 19, 2018

Feature request: Set user agent from viewport plugin #3223

Closed

mshaaban088 added 9 commits March 20, 2018 00:50

Move manager related files to manager directory

28f75ec

Move addon IDs to shared directory

e2daecf

Decouple initial viewports from SelectViewport component

7ad2e8c

Add responsive viewport and make it default

39ca2b1

Register all channel listeners on componentDidMount

5a2c5a0

Add custom viewports to the official-storybook example

29c81a7

Update viewport addon documentation

8a0d5b4

Allow setting custom default viewport per story

d304659

Fix linting issues

mshaaban088 force-pushed the master branch from 63d11ce to d304659 Compare March 19, 2018 23:53

Merge branch 'master' into master

af02d9a

Hypnosphi merged commit b2f0115 into storybookjs:master Mar 20, 2018

Hypnosphi mentioned this pull request Aug 2, 2018

Add additional device options to addon-viewport #3918

Merged

dependencies bot mentioned this pull request Jan 15, 2021

Update 25 dependencies from npm #2724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

viewport-addon: Make the addon configurable #3099

viewport-addon: Make the addon configurable #3099

mshaaban088 commented Feb 25, 2018 •

edited

Loading

igor-dv left a comment

igor-dv Feb 26, 2018

Hypnosphi Feb 26, 2018

mshaaban088 Feb 26, 2018

igor-dv Feb 26, 2018

mshaaban088 Feb 26, 2018

igor-dv Feb 26, 2018

mshaaban088 Feb 26, 2018

codecov bot commented Feb 26, 2018 •

edited

Loading

mshaaban088 commented Feb 26, 2018

Hypnosphi Feb 26, 2018

mshaaban088 Feb 26, 2018

mshaaban088 Feb 26, 2018

Hypnosphi left a comment

mshaaban088 commented Feb 26, 2018

igor-dv commented Feb 27, 2018

Hypnosphi commented Feb 27, 2018 •

edited

Loading

mshaaban088 commented Feb 27, 2018

Hypnosphi commented Feb 27, 2018

mshaaban088 commented Mar 11, 2018 •

edited

Loading

Hypnosphi left a comment

Hypnosphi commented Mar 11, 2018

mshaaban088 commented Mar 12, 2018

Hypnosphi commented Mar 12, 2018

mshaaban088 commented Mar 17, 2018

Hypnosphi commented Mar 19, 2018

mshaaban088 commented Mar 19, 2018

mshaaban088 commented Mar 19, 2018

viewport-addon: Make the addon configurable #3099

viewport-addon: Make the addon configurable #3099

Conversation

mshaaban088 commented Feb 25, 2018 • edited Loading

Edit

What I did

Add new device

Use new list of devices

Change the default viewport

Change the default viewport per story

How to test

igor-dv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov bot commented Feb 26, 2018 • edited Loading

Codecov Report

mshaaban088 commented Feb 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hypnosphi left a comment

Choose a reason for hiding this comment

mshaaban088 commented Feb 26, 2018

igor-dv commented Feb 27, 2018

Hypnosphi commented Feb 27, 2018 • edited Loading

mshaaban088 commented Feb 27, 2018

Hypnosphi commented Feb 27, 2018

mshaaban088 commented Mar 11, 2018 • edited Loading

Hypnosphi left a comment

Choose a reason for hiding this comment

Hypnosphi commented Mar 11, 2018

mshaaban088 commented Mar 12, 2018

Hypnosphi commented Mar 12, 2018

mshaaban088 commented Mar 17, 2018

Hypnosphi commented Mar 19, 2018

mshaaban088 commented Mar 19, 2018

mshaaban088 commented Mar 19, 2018

mshaaban088 commented Feb 25, 2018 •

edited

Loading

codecov bot commented Feb 26, 2018 •

edited

Loading

Hypnosphi commented Feb 27, 2018 •

edited

Loading

mshaaban088 commented Mar 11, 2018 •

edited

Loading