-
-
Notifications
You must be signed in to change notification settings - Fork 9.4k
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
Mobile device view: toggling stories panel with ☰ button #3337
Changes from 6 commits
ce32102
95cf942
b434515
a92759d
750a699
a65ef02
f7756fa
0773a14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,54 @@ | ||
// Jest Snapshot v1, https://goo.gl/fbAQLP | ||
|
||
exports[`Storyshots ui/stories/Header mobile-view 1`] = ` | ||
<div | ||
style="background:none;margin:10px 0;display:flex" | ||
> | ||
<button | ||
style="text-transform:uppercase;font-size:12px;font-weight:bolder;color:rgb(130, 130, 130);border:1px solid rgb(193, 193, 193);text-align:center;border-radius:2px;cursor:pointer;display:inlineBlock;padding:0;margin:0 15px;background-color:inherit;outline:0;width:30px;flex-shrink:0;padding-bottom:2px" | ||
> | ||
☰ | ||
</button> | ||
<a | ||
href="http://google.com" | ||
rel="noopener noreferrer" | ||
style="text-decoration:none;flex-grow:1;display:flex;align-items:center;justify-content:center;border:none;border-radius:2px" | ||
target="_blank" | ||
> | ||
<h3 | ||
style="font-family:-apple-system, \\".SFNSText-Regular\\", \\"San Francisco\\", BlinkMacSystemFont, \\"Segoe UI\\", \\"Roboto\\", \\"Oxygen\\", \\"Ubuntu\\", \\"Cantarell\\", \\"Fira Sans\\", \\"Droid Sans\\", \\"Helvetica Neue\\", \\"Lucida Grande\\", \\"Arial\\", sans-serif;color:#828282;-webkit-font-smoothing:antialiased;text-transform:uppercase;letter-spacing:1.5px;font-size:12px;font-weight:bolder;text-align:center;cursor:pointer;padding:5px;margin:0;overflow:hidden" | ||
> | ||
name | ||
</h3> | ||
</a> | ||
<button | ||
style="text-transform:uppercase;font-size:12px;font-weight:bolder;color:rgb(130, 130, 130);border:1px solid rgb(193, 193, 193);text-align:center;border-radius:2px;cursor:pointer;display:inlineBlock;padding:0;margin:0 15px;background-color:inherit;outline:0;width:30px;flex-shrink:0" | ||
> | ||
⌘ | ||
</button> | ||
</div> | ||
`; | ||
|
||
exports[`Storyshots ui/stories/Header simple 1`] = ` | ||
<div | ||
style="background:#F7F7F7;margin:0 0 10px;display:flex" | ||
> | ||
<a | ||
href="http://google.com" | ||
rel="noopener noreferrer" | ||
style="text-decoration:none;flex-grow:1;display:flex;align-items:center;justify-content:center;border:1px solid rgb(193, 193, 193);border-radius:2px" | ||
target="_blank" | ||
> | ||
<h3 | ||
style="font-family:-apple-system, \\".SFNSText-Regular\\", \\"San Francisco\\", BlinkMacSystemFont, \\"Segoe UI\\", \\"Roboto\\", \\"Oxygen\\", \\"Ubuntu\\", \\"Cantarell\\", \\"Fira Sans\\", \\"Droid Sans\\", \\"Helvetica Neue\\", \\"Lucida Grande\\", \\"Arial\\", sans-serif;color:#828282;-webkit-font-smoothing:antialiased;text-transform:uppercase;letter-spacing:1.5px;font-size:12px;font-weight:bolder;text-align:center;cursor:pointer;padding:5px;margin:0;overflow:hidden" | ||
> | ||
name | ||
</h3> | ||
</a> | ||
<button | ||
style="text-transform:uppercase;font-size:12px;font-weight:bolder;color:rgb(130, 130, 130);border:1px solid rgb(193, 193, 193);text-align:center;border-radius:2px;cursor:pointer;display:inlineBlock;padding:0;margin:0 0 0 5px;background-color:inherit;outline:0;width:30px;flex-shrink:0" | ||
> | ||
⌘ | ||
</button> | ||
</div> | ||
`; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,11 @@ import PropTypes from 'prop-types'; | |
import React from 'react'; | ||
import { baseFonts } from '@storybook/components'; | ||
|
||
const wrapperStyle = { | ||
background: '#F7F7F7', | ||
marginBottom: 10, | ||
const wrapperStyle = isMobileDevice => ({ | ||
background: isMobileDevice ? 'none' : '#F7F7F7', | ||
margin: isMobileDevice ? '10px 0' : '0 0 10px', | ||
display: 'flex', | ||
}; | ||
}); | ||
|
||
const headingStyle = { | ||
...baseFonts, | ||
|
@@ -22,7 +22,7 @@ const headingStyle = { | |
overflow: 'hidden', | ||
}; | ||
|
||
const shortcutIconStyle = { | ||
const iconStyle = isMobileDevice => ({ | ||
textTransform: 'uppercase', | ||
fontSize: 12, | ||
fontWeight: 'bolder', | ||
|
@@ -33,44 +33,58 @@ const shortcutIconStyle = { | |
cursor: 'pointer', | ||
display: 'inlineBlock', | ||
padding: 0, | ||
margin: '0 0 0 5px', | ||
margin: isMobileDevice ? '0 15px' : '0 0 0 5px', | ||
backgroundColor: 'inherit', | ||
outline: 0, | ||
width: 30, | ||
flexShrink: 0, | ||
}); | ||
|
||
const burgerIconStyle = { | ||
...iconStyle(true), | ||
paddingBottom: 2, | ||
}; | ||
|
||
const linkStyle = { | ||
const linkStyle = isMobileDevice => ({ | ||
textDecoration: 'none', | ||
flexGrow: 1, | ||
display: 'flex', | ||
alignItems: 'center', | ||
justifyContent: 'center', | ||
border: '1px solid rgb(193, 193, 193)', | ||
border: isMobileDevice ? 'none' : '1px solid rgb(193, 193, 193)', | ||
borderRadius: 2, | ||
}; | ||
}); | ||
|
||
const Header = ({ openShortcutsHelp, name, url }) => ( | ||
<div style={wrapperStyle}> | ||
<a style={linkStyle} href={url} target="_blank" rel="noopener noreferrer"> | ||
const Header = ({ openShortcutsHelp, onBurgerButtonClick, name, url, isMobileDevice }) => ( | ||
<div style={wrapperStyle(isMobileDevice)}> | ||
{isMobileDevice && ( | ||
<button style={burgerIconStyle} onClick={onBurgerButtonClick}> | ||
☰ | ||
</button> | ||
)} | ||
<a style={linkStyle(isMobileDevice)} href={url} target="_blank" rel="noopener noreferrer"> | ||
<h3 style={headingStyle}>{name}</h3> | ||
</a> | ||
<button style={shortcutIconStyle} onClick={openShortcutsHelp}> | ||
<button style={iconStyle(isMobileDevice)} onClick={openShortcutsHelp}> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this button is useless on mobiles, it shows keyboard shortcuts There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that some people use USB keyboards with mobile devices, so I wouldn't necessarily just remove it because of mobile. |
||
⌘ | ||
</button> | ||
</div> | ||
); | ||
|
||
Header.defaultProps = { | ||
openShortcutsHelp: null, | ||
onBurgerButtonClick: null, | ||
name: '', | ||
url: '', | ||
isMobileDevice: false, | ||
}; | ||
|
||
Header.propTypes = { | ||
openShortcutsHelp: PropTypes.func, | ||
onBurgerButtonClick: PropTypes.func, | ||
name: PropTypes.string, | ||
url: PropTypes.string, | ||
isMobileDevice: PropTypes.bool, | ||
}; | ||
|
||
export default Header; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
import React from 'react'; | ||
import { storiesOf } from '@storybook/react'; | ||
import { action } from '@storybook/addon-actions'; | ||
|
||
import Header from './header'; | ||
|
||
const openShortcutsHelp = action('openShortcutsHelp'); | ||
storiesOf('ui/stories/Header', module) | ||
.add('simple', () => ( | ||
<Header name="name" url="http://google.com" openShortcutsHelp={openShortcutsHelp} /> | ||
)) | ||
.add('mobile-view', () => ( | ||
<Header | ||
name="name" | ||
url="http://google.com" | ||
openShortcutsHelp={openShortcutsHelp} | ||
isMobileDevice | ||
/> | ||
)); |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -98,18 +98,22 @@ const defaultSizes = { | |
}, | ||
}; | ||
|
||
const saveSizes = sizes => { | ||
const saveSizes = (sizes, isMobileDevice) => { | ||
const storageItemName = isMobileDevice ? 'panelSizesMobile' : 'panelSizes'; | ||
|
||
try { | ||
localStorage.setItem('panelSizes', JSON.stringify(sizes)); | ||
localStorage.setItem(storageItemName, JSON.stringify(sizes)); | ||
return true; | ||
} catch (e) { | ||
return false; | ||
} | ||
}; | ||
|
||
const getSavedSizes = sizes => { | ||
const getSavedSizes = (sizes, isMobileDevice) => { | ||
const storageItemName = isMobileDevice ? 'panelSizesMobile' : 'panelSizes'; | ||
|
||
try { | ||
const panelSizes = localStorage.getItem('panelSizes'); | ||
const panelSizes = localStorage.getItem(storageItemName); | ||
if (panelSizes) { | ||
return JSON.parse(panelSizes); | ||
} | ||
|
@@ -125,7 +129,7 @@ class Layout extends React.Component { | |
constructor(props) { | ||
super(props); | ||
|
||
this.layerSizes = getSavedSizes(defaultSizes); | ||
this.layerSizes = getSavedSizes(defaultSizes, props.isMobileDevice); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks like this second argument isn't used anymore There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yeah, you are right ) please advise how could I clean it up? |
||
|
||
this.state = { | ||
previewPanelDimensions: { | ||
|
@@ -159,7 +163,7 @@ class Layout extends React.Component { | |
onResize(pane, mode) { | ||
return size => { | ||
this.layerSizes[pane][mode] = size; | ||
saveSizes(this.layerSizes); | ||
saveSizes(this.layerSizes, this.props.isMobileDevice); | ||
|
||
const { clientWidth, clientHeight } = this.previewPanelRef; | ||
|
||
|
@@ -177,13 +181,15 @@ class Layout extends React.Component { | |
goFullScreen, | ||
showStoriesPanel, | ||
showAddonPanel, | ||
addonPanelInRight, | ||
addonPanel, | ||
storiesPanel, | ||
preview, | ||
isMobileDevice, | ||
} = this.props; | ||
const { previewPanelDimensions } = this.state; | ||
|
||
const addonPanelInRight = isMobileDevice ? false : this.props.addonPanelInRight; | ||
|
||
const storiesPanelOnTop = false; | ||
|
||
let previewStyle = normalPreviewStyle; | ||
|
@@ -192,7 +198,7 @@ class Layout extends React.Component { | |
previewStyle = fullScreenPreviewStyle; | ||
} | ||
|
||
const sizes = getSavedSizes(this.layerSizes); | ||
const sizes = getSavedSizes(this.layerSizes, isMobileDevice); | ||
|
||
const storiesPanelDefaultSize = !storiesPanelOnTop | ||
? sizes.storiesPanel.left | ||
|
@@ -209,7 +215,7 @@ class Layout extends React.Component { | |
<SplitPane | ||
split={storiesSplit} | ||
allowResize={showStoriesPanel} | ||
minSize={150} | ||
minSize={isMobileDevice ? 0 : 150} | ||
maxSize={-400} | ||
size={showStoriesPanel ? storiesPanelDefaultSize : 1} | ||
defaultSize={storiesPanelDefaultSize} | ||
|
@@ -271,6 +277,11 @@ Layout.propTypes = { | |
preview: PropTypes.func.isRequired, | ||
addonPanel: PropTypes.func.isRequired, | ||
addonPanelInRight: PropTypes.bool.isRequired, | ||
isMobileDevice: PropTypes.bool, | ||
}; | ||
|
||
Layout.defaultProps = { | ||
isMobileDevice: false, | ||
}; | ||
|
||
export default Layout; |
This file was deleted.
This file was deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@igor-dv I prefer this:
![](https://user-images.githubusercontent.com/3757759/38176267-ac776684-35f4-11e8-9bd0-2c486937f02e.png)
both visually and UX, you should not overlay anything over the preview.