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.

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

Keyboard shortcuts and fuzzy search #141

Merged
merged 13 commits into from Apr 25, 2016
Merged

Keyboard shortcuts and fuzzy search #141

merged 13 commits into from Apr 25, 2016

Conversation

ritz078
Copy link
Contributor

@ritz078 ritz078 commented Apr 22, 2016

2016-04-23 00_22_07

Need to add some tests.

Current key bindings:
Ctrl + Shift + O => Open search box
Ctrl + Shift + L => Toggle left panel
Ctrl + Shift + B => Toggle bottom panel
Ctrl + Shift + F => Toggle Full screen view

Closes #59 and #121

@arunoda
Copy link
Member

arunoda commented Apr 22, 2016

Okay. This is super.
One just UI change. Can you keyboard icon next to the logo and show the keymap.

@arunoda
Copy link
Member

arunoda commented Apr 22, 2016

Shall we keep the current filter box as it is.

@@ -194,4 +217,5 @@ StorybookControls.propTypes = {
selectedStory: React.PropTypes.string,
onKind: React.PropTypes.func,
onStory: React.PropTypes.func,
syncedStore: React.PropTypes.object,
Copy link
Member

Choose a reason for hiding this comment

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

Hey, is there anyway we can use a function rather not passing the syncedStore.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes , we can. will do it.

@arunoda
Copy link
Member

arunoda commented Apr 22, 2016

@mnmtanish Could you go on this as well.
It's always better if we some fresh eyes at this.

@arunoda
Copy link
Member

arunoda commented Apr 23, 2016

@A few ideas:

  • Shall we use 'CMD' + 'SHIFT' in mac. Just like in Atom, ST2/3
  • Shall we use 'CMD + SHIFT + P' for lookup just like we used to do in Editors
  • In the full screen mode, still there's a margin around the preview. I hope we can remove that.
  • Add relevant metatags to support the full screen mode. (I mean to use browser tools to try with different viewport sizes)

I think, we can break this PR in multiple and then we can easily review this and pull into core. What do you think of dividing this PR into three:

  1. Basic Keyboard Support (the logistics)
  2. Full screen support + other minor keyboard command
  3. Search box

Then we can review individual components and take them in quicker.

@ritz078 What do you think?

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 23, 2016

I will definitely add CMD support. And regarding separate PRs each functionality is dependent on other. If i remove the full screen support , it will just remove 10 lines from code . So i will suggest it is kept this way. I still have refactorings and improvements to do. So I have already written WIP in the title. Keep your ideas coming.

Edit : I am removing the full screen part from this PR.

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 23, 2016

There's no point in keeping a separate search box which can't search through stories. The two search boxes will differ in behaviour and that's not good. Even the editors don't have separate search bars because on small screens it takes considerable space. But I may be wrong.

@arunoda
Copy link
Member

arunoda commented Apr 23, 2016

Okay. Let's remove other shortcuts from this as well.
Just work on the search.

Having the filter box is pretty important. Here's why:

We have a lot of stories (and a lot of kinds) in one of our app.
We need to use the left navigation panel. We are not looking for a jump to story functionality.
That's why we need the filter box.

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 23, 2016

I have already fixed all the other shortcuts so why remove them now ?

@arunoda
Copy link
Member

arunoda commented Apr 23, 2016

Ah okay. Then fine.

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 23, 2016

You can even search for a kind and the results of that kind will come at a top with the name of the kind written to the right. Eventually, you land at a story anyways. Till now we were landing at the first story by default. There's nothing like a separate page for kind.

@arunoda
Copy link
Member

arunoda commented Apr 23, 2016

Yes. true.
But that's not the case.
We don't need to search for every time we need to switch stories.
Just want to browse through the left panal.

Sometimes it's not just a single kind but some similar.

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 23, 2016

Cool. I will restore the filter .

@arunoda
Copy link
Member

arunoda commented Apr 23, 2016

Let's keep the filter as it is.
If we think the jump to story is better after sometimes using it, let's remove the filter.
For now, let's keep it.

@arunoda
Copy link
Member

arunoda commented Apr 23, 2016

Ha ha. You reply faster.
Cool. Thanks.

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 23, 2016

A suggestion for future : We can keep both kind and story filter. If you add a prefix like @ in search , you only filter the kinds and by default stories.

@arunoda
Copy link
Member

arunoda commented Apr 23, 2016

@ritz078 That's good.

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 24, 2016

@arunoda have a look now.

@arunoda
Copy link
Member

arunoda commented Apr 24, 2016

Cool. Will do a review tommorow morning for sure.

@arunoda
Copy link
Member

arunoda commented Apr 25, 2016

Okay. This looks great.
But somehow, CMD + SHIFT + B is not working but CTRL + SHIFT + B is working.
Once you fixed it, I think we could merge it.

Note: Our UI is now kind a complex and it's with some adhoc design. I'm moving the codebase to Mantra specification within next 2 days. After that we may need to rework some stuff. (Possibly more testing)

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 25, 2016

If only CMD+SHIFT+B is not working then maybe some other software that is open uses that shortcut. The solution will be to change the shortcut but anyways no shortcut is safe.

Which browser and OS are you using ?

@ritz078 ritz078 changed the title Keyboard shortcuts and fuzzy search <WIP> Keyboard shortcuts and fuzzy search Apr 25, 2016
@arunoda
Copy link
Member

arunoda commented Apr 25, 2016

I'm using Chrome on Mac.

@ritz078
Copy link
Contributor Author

ritz078 commented Apr 25, 2016

I have checked and its working fine on chrome. Definitely, some other app is using that shortcut on app level.

I can change the shortcut but that may fail in someone else's computer if that is being used by some other software.

@arunoda
Copy link
Member

arunoda commented Apr 25, 2016

That's fine. Let's keep it like this for now.

@arunoda
Copy link
Member

arunoda commented Apr 25, 2016

I'll take this.

@arunoda arunoda merged commit ab52c45 into storybookjs:master Apr 25, 2016
@arunoda
Copy link
Member

arunoda commented Apr 25, 2016

Published this as: :v1.18.0

@ritz078 ritz078 deleted the feat/fuzzy-search branch April 26, 2016 02:32
@arunoda
Copy link
Member

arunoda commented May 5, 2016

Hi @ritz078,

I re-wrote the whole storybook again for a modular release. (did with `v1.20.0)
So, I couldn't get your code for fuzzy-search for that.
I had to do a release soon. (I don't like to ask other's PR to wait for this refactor)

I'm planning to do it by next-week. If you've some time I'm looking for your help.
Here's a doc about new Storybook Internals.

@ritz078
Copy link
Contributor Author

ritz078 commented May 5, 2016

No problem. I am developing a separate fuzzy search component named react-fuzzy-search . I will integrate it once the tests for that are completed.

@arunoda
Copy link
Member

arunoda commented May 5, 2016

@ritz078 that'sweet.

@shilman shilman added the misc label May 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants