Mobile device view: toggling stories panel with ☰ button #3337

denzenin · 2018-04-01T18:38:06Z

Issue:

What I did

adapted Header component to have ☰ button when on mobile device
make stories-panel toggled by clicking ☰ button
added helper to detect whether the storybook is open on mobile device (this is done through userAgent of window)
adjusted styles

Now on mobile devices the layout looks something like this when storybook is rendered:

How to test

Open storybook on mobile device

Is this testable with Jest or Chromatic screenshots?
yes (I added some new tests)
Does this need a new example in the kitchen sink apps?
no
Does this need an update to the documentation?
not sure
If your answer is yes to any of these, please make sure to include it in your PR.

* add separate Header component * add isMobileDevice helper * initially place addonPanel at the bottom if storybook is open on mobile device * add first part of tests

Hypnosphi · 2018-04-01T18:53:02Z

lib/ui/src/modules/ui/libs/is_mobile_device.js

@@ -0,0 +1,7 @@
+export default userAgent => {
+  if (/Android|webOS|iPhone|iPad|iPod|BlackBerry|IEMobile|Opera Mini/i.test(userAgent)) {


Do we really need javascript for this? Can we just show/hide things using media queries instead?

I did it to avoid changes to layout in general.
Depending on whether it is mobile device, the header is rendered outside of stories-panel as a standalone component and visibility of stories-panel is just toggled with the existing mechanism (to hide when app first loaded on mobile device) :)

OK makes sense

I'm in the media query camp as well. If on desktop, a user scales the window smaller so they can code and render in the browser side by side, they should be able to use the mobile view as well.

@danielduan with current version of Storybook there is nothing stopping users from hiding stories-panel on desktop (with shortcuts) and then resize the browser window as it is needed for your particular case. I want to emphasize that this pull request solves the problem with testing components on physical mobile devices :)
And I tried to affect as little of desktop version as possible on purpose. Cause this is a whole separate task related to layouts in general.

I understand that you don't want to expand the scope of this change so this sounds fine with me.

It's still a broader issue that we'll need to address at some point.

Let's take it 1 step at a time? I don't fancy userAgent sniffing either, but it does solve an issue and down-scopes this PR well.

Let's improve the manager head for desktop after.

Hypnosphi · 2018-04-01T19:36:40Z

lib/ui/src/modules/ui/components/header.js

      <h3 style={headingStyle}>{name}</h3>
    </a>
-    <button style={shortcutIconStyle} onClick={openShortcutsHelp}>
+    <button style={iconStyle(isMobileDevice)} onClick={openShortcutsHelp}>


Looks like this button is useless on mobiles, it shows keyboard shortcuts

We can actually make those things clickable and perform the action.. but once the left panel is gone (either by toggling it, or by going full-screen, you can't go back)

Note that some people use USB keyboards with mobile devices, so I wouldn't necessarily just remove it because of mobile.

Hypnosphi · 2018-04-01T19:39:21Z

lib/ui/src/modules/ui/components/layout/index.js

@@ -98,7 +98,11 @@ const defaultSizes = {
  },
 };

-const saveSizes = sizes => {
+const saveSizes = (sizes, isMobileDevice) => {
+  if (isMobileDevice) {


Please add some inline comment explaining why do we need to skip saving here

I see two ways of refactoring this part:

Extract the saveSizes and getSavedSizes to a separate file and do not change their functionality.
Change only the Layout component to be coupled to the isMobileDevice.

As an alternative, you can store mobile sizes in a sperate entry of the storage.

@igor-dv I like the second option

Sorry, I don't really get the point of separate storage entries. Different devices cannot share the same localStorage AFAIK

@Hypnosphi initially I avoided saving dimensions on mobile as the resizing worked not perfectly when I firstly tried it (I did not face issues that you have found later, but still resizing seemed to be a hard task to perform. So I wanted to avoid situations when you somehow resized the pane and did not like the results but the new unwanted sizes are saved in localStorage, so you cannot reset the layout by refreshing the page.

Maybe I misunderstood Igor's comment above about separation of saved dimensions, if it is not needed, but we still want to save sizes on mobile – let's just drop the changes and leave this part as it is in master right now?

I talked more about the browser emulators. I think in this case the storage is shared.

Hypnosphi · 2018-04-01T19:40:47Z

lib/ui/src/modules/ui/components/layout/index.js

    } = this.props;
    const { previewPanelDimensions } = this.state;

+    const addonPanelInRight = isMobileDevice ? 0 : this.props.addonPanelInRight;


Why number not boolean?

missed it, thanks :)

Hypnosphi · 2018-04-01T19:51:36Z

It breaks in a weird manner when I try to resize stories panel on mobile: https://deploy-preview-3337--storybooks-official.netlify.com/?selectedKind=ui%2FAddonPanel&selectedStory=default&full=0&addons=1&stories=0&panelRight=0&addonPanel=storybook%2Factions%2Factions-panel

ndelangen · 2018-04-01T23:41:04Z

@denzenin Thank you for this amazing first PR, thank you!

igor-dv · 2018-04-02T10:24:29Z

lib/ui/src/modules/ui/components/header.js

+const Header = ({ openShortcutsHelp, onBurgerButtonClick, name, url, isMobileDevice }) => (
+  <div style={wrapperStyle(isMobileDevice)}>
+    {isMobileDevice && (
+      <button style={burgerIconStyle} onClick={onBurgerButtonClick}>


Why don't just show this button always? I don't see any cons to it in a web mode, and having it in both modes will reduce complexity.

for me, when I see a ☰ button on mobile websites, I expect it to "always be there for me" :)
in this particular case, if we leave it on Desktop version and have the same behaviour (toggling story-panel, it will be hidden once story-panel is hidden)

but I have no issue with keeping the button, so if you advise to get rid of the check above – I'll remove it

Maybe we can have something like this when it's closed (in both modes):

The floating button (with opacity). It will open the stories panel with the header in it. This way we can reuse most of the UI between web and mweb.

I assume there is a disadvantage in mobile view with floating button. In case of this floating element you will have to always keep it in mind when writing stories of the components. We would need to add some additional containers/spacing in stories to test the component without visual/touch interference with floating element. In my opinion header is OK and quite simple solution for this.

@igor-dv I prefer this:

both visually and UX, you should not overlay anything over the preview.

igor-dv · 2018-04-02T10:41:03Z

lib/ui/src/modules/ui/components/layout/index.js

@@ -98,7 +98,11 @@ const defaultSizes = {
  },
 };

-const saveSizes = sizes => {
+const saveSizes = (sizes, isMobileDevice) => {
+  if (isMobileDevice) {


I see two ways of refactoring this part:

Extract the saveSizes and getSavedSizes to a separate file and do not change their functionality.
Change only the Layout component to be coupled to the isMobileDevice.

As an alternative, you can store mobile sizes in a sperate entry of the storage.

denzenin · 2018-04-02T18:01:11Z

@Hypnosphi regarding breaking on resize. yes, it is weird indeed. It seems that resizing of SplitPane works far from smooth on mobile. Tried it on master branch. Even when I had succeeded to resize story-panel I could resize it again (after first resize) only once in a bunch of tries.

denzenin · 2018-04-02T18:06:59Z

@ndelangen thank you for the kind words :)

codecov · 2018-04-02T18:10:25Z

Codecov Report

Merging #3337 into master will decrease coverage by 0.01%.
The diff coverage is 42.68%.

@@            Coverage Diff             @@
##           master    #3337      +/-   ##
==========================================
- Coverage   37.21%   37.19%   -0.02%     
==========================================
  Files         463      465       +2     
  Lines       10241    10300      +59     
  Branches      914      923       +9     
==========================================
+ Hits         3811     3831      +20     
- Misses       5892     5906      +14     
- Partials      538      563      +25

Impacted Files	Coverage Δ
...i/src/modules/ui/components/stories_panel/index.js	`20.63% <0%> (-1.04%)`	⬇️
lib/ui/src/modules/ui/routes.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/ui/components/header.stories.js	`100% <100%> (ø)`
addons/notes/src/index.js	`90% <100%> (ø)`	⬆️
...dules/ui/components/stories_panel/index.stories.js	`91.66% <100%> (+0.75%)`	⬆️
lib/ui/src/modules/ui/containers/stories_panel.js	`25% <16.66%> (-1.32%)`	⬇️
lib/ui/src/modules/ui/containers/header.js	`27.58% <33.33%> (ø)`
lib/ui/src/modules/ui/components/header.js	`25.71% <42.85%> (ø)`
lib/ui/src/modules/ui/containers/layout.js	`20% <44.44%> (+7.5%)`	⬆️
lib/ui/src/modules/ui/components/layout/index.js	`29.33% <50%> (+1.14%)`	⬆️
... and 88 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 a504a08...0773a14. Read the comment docs.

Hypnosphi · 2018-04-03T12:11:14Z

So should we maybe disable resizing on mobile at all?

denzenin · 2018-04-10T07:14:39Z

Good day to all, could you please advise what changes should I make to my pull request to move it forward? Maybe one step at a time but still I need to know those steps :)

Hypnosphi · 2018-04-10T09:45:23Z

What do you think about this?

should we maybe disable resizing on mobile at all?

You also need to resolve conflicts, feel free to ask for help if you need one

denzenin · 2018-04-10T18:31:36Z

@Hypnosphi I am totally ok with that. If it's ok with you and @igor-dv – will do as it was initially in PR:

const saveSizes = (sizes, isMobileDevice) => {
  if (isMobileDevice) {
    return true;
  }
...
}

const getSavedSizes = (sizes, isMobileDevice) => {
  if (isMobileDevice) {
    return sizes;
  }
...
}

and will add some inline comments about why the sizes are not saved/loaded for mobile.

Hypnosphi · 2018-04-10T21:00:43Z

It would be more explicit if we would just pass allowResize={false} on mobile

ndelangen · 2018-04-14T17:21:31Z

Would love to see this merged soon! 🙇

denzenin · 2018-04-17T19:21:06Z

@Hypnosphi good evening. Fixed the resizing as you proposed.
Also fixed conflicts and would appreciate if you have a look at the merge commit.

Hypnosphi · 2018-04-17T19:55:11Z

lib/ui/src/modules/ui/components/layout/index.js

@@ -130,7 +130,7 @@ class Layout extends React.Component {
  constructor(props) {
    super(props);

-    this.layerSizes = getSavedSizes(defaultSizes);
+    this.layerSizes = getSavedSizes(defaultSizes, props.isMobileDevice);


Looks like this second argument isn't used anymore

yeah, you are right ) please advise how could I clean it up?

ndelangen · 2018-04-17T20:59:54Z

Awesome work @denzenin ! 💪

denzenin · 2018-04-24T05:27:09Z

yay! thank you all for your help and support :)

Aarbel · 2018-08-26T18:17:26Z

hi ! in which storybook version this PR has been included ? thanks !

Hypnosphi · 2018-09-01T17:52:22Z

@Aarbel try latest alpha, 4.0.0-alpha.20

denzenin added 4 commits March 24, 2018 16:53

Add different header and panels layout for mobile devices:

ce32102

* add separate Header component * add isMobileDevice helper * initially place addonPanel at the bottom if storybook is open on mobile device * add first part of tests

Fix tests

95cf942

Add mobile-view story for header

b434515

Fix stories-panel: still need to pass url and name via props

a92759d

denzenin requested review from Hypnosphi, igor-dv, ndelangen, tmeasday and usulpro as code owners April 1, 2018 18:38

denzenin mentioned this pull request Apr 1, 2018

Mobile storybook view #124

Closed

Hypnosphi added ui react-native feature request and removed react-native labels Apr 1, 2018

Hypnosphi requested changes Apr 1, 2018

View reviewed changes

Hypnosphi mentioned this pull request Apr 1, 2018

Trigger knobs in fullscreen #2905

Closed

Hypnosphi reviewed Apr 1, 2018

View reviewed changes

igor-dv reviewed Apr 2, 2018

View reviewed changes

Fix saving sizes to local storage. Use boolean instead of number

750a699

Add story for stories-panel: when on mobile device

a65ef02

ndelangen approved these changes Apr 3, 2018

View reviewed changes

denzenin added 2 commits April 17, 2018 20:14

Forbid resizing of SplitPane for mobile devices

f7756fa

Merge master

0773a14

denzenin requested a review from alexandrebodin as a code owner April 17, 2018 19:14

Hypnosphi approved these changes Apr 17, 2018

View reviewed changes

Hypnosphi reviewed Apr 17, 2018

View reviewed changes

Hypnosphi merged commit b6178a2 into storybookjs:master Apr 17, 2018

jackkoppa mentioned this pull request Jan 2, 2019

Mobile layout for storybook version 4 #4692

Closed

		@@ -0,0 +1,7 @@
		export default userAgent => {
		if (/Android\|webOS\|iPhone\|iPad\|iPod\|BlackBerry\|IEMobile\|Opera Mini/i.test(userAgent)) {

Mobile device view: toggling stories panel with ☰ button #3337

Mobile device view: toggling stories panel with ☰ button #3337

Conversation

denzenin commented Apr 1, 2018

What I did

How to test

Choose a reason for hiding this comment

denzenin Apr 1, 2018 • edited

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

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

Hypnosphi Apr 3, 2018 • edited

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

Hypnosphi commented Apr 1, 2018

ndelangen commented Apr 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denzenin Apr 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen Apr 3, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

denzenin commented Apr 2, 2018

denzenin commented Apr 2, 2018

codecov bot commented Apr 2, 2018 • edited

Codecov Report

Hypnosphi commented Apr 3, 2018

denzenin commented Apr 10, 2018

Hypnosphi commented Apr 10, 2018

denzenin commented Apr 10, 2018

Hypnosphi commented Apr 10, 2018 • edited

ndelangen commented Apr 14, 2018

denzenin commented Apr 17, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ndelangen commented Apr 17, 2018

denzenin commented Apr 24, 2018

Aarbel commented Aug 26, 2018 • edited

Hypnosphi commented Sep 1, 2018

denzenin Apr 1, 2018 •

edited

Hypnosphi Apr 3, 2018 •

edited

denzenin Apr 3, 2018 •

edited

ndelangen Apr 3, 2018 •

edited

codecov bot commented Apr 2, 2018 •

edited

Hypnosphi commented Apr 10, 2018 •

edited

Aarbel commented Aug 26, 2018 •

edited