Skip to content
This repository has been archived by the owner on Nov 10, 2017. It is now read-only.

Add Floating Menu #69

Closed
wants to merge 8 commits into from
Closed

Add Floating Menu #69

wants to merge 8 commits into from

Conversation

usulpro
Copy link
Member

@usulpro usulpro commented Jan 10, 2017

It implements #56

I moved all items from 'keyboard shortcuts' to menu wich avalibale in the left panel. And when the left panel is hidden it floats in the preview area. It also has buttons to browse through stories. It gives access to Storybook core functionalities both by a mouse and a touchscreen from normal and fullscreen view.

here is a demo page to check it on mobile device.

@arunoda
Copy link
Contributor

arunoda commented Jan 11, 2017

This is great. Here's a list of common UI suggestions.

  • Add a way to show our existing keymap window. (Add a new section called Keyboard shortcuts in the menu)
  • I wanna see this on the top in the normal view: http://recordit.co/499hST1YVy
  • I am not a fan of the transition when opening the page. Shall we remove it or make it very speed.

@@ -0,0 +1,165 @@
/* eslint-disable */
Copy link
Contributor

Choose a reason for hiding this comment

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

Can't we get this via NPM?

Copy link
Member Author

Choose a reason for hiding this comment

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

It's just a bundle with svg images which I use in my menu. I have two reasons to put them into one file:

  • to don't bloat src with a bunch of image files

  • to don't add new dependency to project (I need here file-loader or base64-image-loader)

So I just prebuild it manually with base64-image-loader. Maybe it could be usefull later if you'll want to extend it with more images? But if you don't like such approach could you suggest the better way how to get this images?

const root = (
<div>
<Layout
leftPanel={() => (<LeftPanel />)}
leftPanel={(menu) => (<LeftPanel floatingMenu={menu} />)}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like getting the menu from a function. Since we define the floatingMenu inside here. Let's use it directly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok I removed it from here

@arunoda
Copy link
Contributor

arunoda commented Jan 11, 2017

@mnmtanish could you review this too?

@thani-sh
Copy link
Contributor

Sure, I'll check it out today evening.

@usulpro
Copy link
Member Author

usulpro commented Jan 11, 2017

I fixed it according your suggestion and updated demo page.

  • I added new menu item "Keyboard shortcuts". But the keymap window is visible only in normal view. How do you think is it Ok? (BTW did you noticed that if you hold mouse over any menu item, the tooltip with keyboard shortcut will be shown?) Another idea to show keyboard shortcuts may be to change menu item names to shortcuts when the mouse is over the "Keyboard shortcuts" menu item.

  • I moved it as you asked and make menu appearing from below (I'll rename 'Footer' to 'Menu' then)

  • I sped up transition to 100ms but if you still don't like it I can remove it complitely

@@ -24,6 +24,21 @@ export function jumpToStory(storyKinds, selectedKind, selectedStory, direction)
};
}

function jumpToKind(storyKinds, selectedKind, selectedStory, direction) {
const currentIndex = storyKinds.findIndex(({ kind }) => (kind === selectedKind));
if (currentIndex === -1) return selectedKind;
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we return both selectedKind and selectedStory ?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure! fixed :)


const linkStyle = {
textDecoration: 'none',
};

const Header = ({ openShortcutsHelp, name, url }) => (
const Header = ({/* openShortcutsHelp,*/ name, url }) => (
Copy link
Contributor

Choose a reason for hiding this comment

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

Just delete unused code, we can always git checkout if we need them again

Copy link
Contributor

Choose a reason for hiding this comment

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

Found some more below, check them too

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok

@@ -1,3 +1,4 @@
node_modules
dist
Copy link
Contributor

Choose a reason for hiding this comment

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

Move this to a new PR, this one needs to be discussed more. Having dist on the repo sucks but we did it anyway so that storybook could be installed from the Github repo

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, no problem!
I just revert it and make npm run prepublish before pushing

PREV_STORY: 7,
SEARCH: 8,
DOWN_PANEL_IN_RIGHT: 9
FULLSCREEN: 'FULLSCREEN',
Copy link
Member Author

Choose a reason for hiding this comment

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

I changed it to strings because it was hard to setup tests for that. Now all test passed so if it matters I can revert it if you want!?

@@ -0,0 +1 @@
export FloatingMenu from './floating_menu';
Copy link
Member Author

Choose a reason for hiding this comment

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

ESLint complains here. Should I disable it here?

@usulpro
Copy link
Member Author

usulpro commented Jan 22, 2017

BTW don't you want to move 'text filter' also into this menu? (or add an item to hide it)
I can add this if you like it

@usulpro
Copy link
Member Author

usulpro commented Feb 16, 2017

@arunoda, @mnmtanish!
Is it possible to merge this?

ndelangen pushed a commit that referenced this pull request Apr 2, 2017
ndelangen pushed a commit that referenced this pull request Apr 2, 2017
Provide storybook context to render
ndelangen pushed a commit that referenced this pull request Apr 10, 2017
ndelangen pushed a commit that referenced this pull request Apr 10, 2017
@ndelangen
Copy link
Contributor

Let's create a PR here: https://github.com/storybooks/storybook

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants