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

Implement filtering on story-level #1432

Merged

Conversation

jackhurley23
Copy link
Contributor

Issue:
#109

What I did

I edited the storyFilter() function to enable filtering on the story-level.
If the "fuzzysearch" matches a storiesOf() level story it will return all stories that are matched.

If the fuzzysearch matches an individual story, it will just return that story.

It will leave the currently active story unfiltered

How to test

See the additions to filters.test.js for examples of it in action.

@shilman shilman requested a review from igor-dv July 8, 2017 07:19
@shilman
Copy link
Member

shilman commented Jul 8, 2017

Nice tests--great work!!!

@codecov
Copy link

codecov bot commented Jul 8, 2017

Codecov Report

Merging #1432 into release/3.2 will increase coverage by 0.09%.
The diff coverage is 57.89%.

Impacted file tree graph

@@              Coverage Diff               @@
##           release/3.2   #1432      +/-   ##
==============================================
+ Coverage        15.91%     16%   +0.09%     
==============================================
  Files              237     237              
  Lines             5047    5054       +7     
  Branches           698     624      -74     
==============================================
+ Hits               803     809       +6     
- Misses            3671    3730      +59     
+ Partials           573     515      -58
Impacted Files Coverage Δ
lib/ui/src/modules/ui/containers/left_panel.js 25% <40%> (-0.72%) ⬇️
lib/ui/src/modules/ui/libs/filters.js 53.33% <64.28%> (+11.66%) ⬆️
lib/ui/src/modules/ui/containers/layout.js 12.5% <0%> (ø) ⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js 6.25% <0%> (ø) ⬆️
addons/events/src/components/Event.js 0% <0%> (ø) ⬆️
app/react-native/src/bin/storybook-build.js 0% <0%> (ø) ⬆️
lib/ui/src/modules/ui/configs/handle_routing.js 28.94% <0%> (ø) ⬆️
addons/info/src/components/PropTable.js 0% <0%> (ø) ⬆️
app/react/src/client/preview/error_display.js 0% <0%> (ø) ⬆️
... and 15 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 fbc665f...a1a0d14. Read the comment docs.

if (fuzzysearch(needle, hstack)) return acc.concat(kindInfo);

// Now search at individual story level and filter results
const matchedStories = kindInfo.stories.filter(story => fuzzysearch(needle, story));
Copy link
Member

Choose a reason for hiding this comment

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

IMO you should add the lowercasing here (like for the kind check, looks like fuzzysearch already do ===, so it maybe redundant) And add tests for this usecase.

Copy link
Member

@ndelangen ndelangen Jul 8, 2017

Choose a reason for hiding this comment

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

Fuzzy search takes care of that, I think?

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

Just checked the fork and now I'm confident that without lowercasing it won't actually work.

@ndelangen
Copy link
Member

ndelangen commented Jul 8, 2017

@shilman did you rebase this on the release branch? I tested this but it's not working as expected I think.

@igor-dv I'm not experiencing any problems with lower/uppercase, i think fuzzy search takes care of that.

@jackhurley23
Copy link
Contributor Author

@ndelangen I can confirm that @igor-dv is correct on the case issue. When filtering at individual story level converting to lower case is required. Its not covered by fuzzysearch.

As mentioned by some users this fork does not seem be working correctly. Any advice on how to get this working is greatly appreciated.

I have it working on local after npm linking a local copy of the repo.
I've attached a GIF demo below:

filteringexample

Copy link
Member

@igor-dv igor-dv left a comment

Choose a reason for hiding this comment

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

Cool.

@igor-dv
Copy link
Member

igor-dv commented Jul 8, 2017

Checked it in lib/ui/example. There is indeed some problem with cra-kitchen-sink linking..
Can it be related to the *.alpha things ?

@jackhurley23
Copy link
Contributor Author

This may need some further work before it's ready. My feeling is that it would be better if the directories expanded to show remaining stories after filtering. Currently it seems hard to predict if react-treebeard will display a result as expanded or not after filtering. I'll look into it further.

@shilman
Copy link
Member

shilman commented Jul 8, 2017

@jhurley23 @igor-dv @ndelangen can confirm that the reason it's working with manual linking but not with bootstrap is due to a versioning problem. I've documented that in issue #1433

If you update the package.json with the following versions and re-run bootstrap, then @jhurley23 's changes show up in cra-kitchen-sink. I'm updating the release/3.2 branch now.

  "devDependencies": {
    "@storybook/addon-actions": "^3.0.0",
    "@storybook/addon-centered": "^3.0.0",
    "@storybook/addon-events": "^3.0.0",
    "@storybook/addon-knobs": "3.2.0-alpha.7",
    "@storybook/addon-info": "^3.0.0",
    "@storybook/addon-links": "3.2.0-alpha.5",
    "@storybook/addon-notes": "3.2.0-alpha.5",
    "@storybook/addon-options": "3.2.0-alpha.5",
    "@storybook/addon-storyshots": "3.2.0-alpha",
    "@storybook/addons": "^3.0.0",
    "@storybook/react": "3.2.0-alpha.7",
    "react-scripts": "1.0.1"
  },

@shilman
Copy link
Member

shilman commented Jul 8, 2017

@jhurley23 agree with you on the expansion behavior. may take some work to get this right!

@ndelangen
Copy link
Member

My patch on lerna would fix that 😭

@shilman
Copy link
Member

shilman commented Jul 15, 2017

@jhurley23 @igor-dv would love to get this in for 3.2. Should I merge as is and we handle the hierarchy expansion behavior in a separate PR (preferably on a branch inside this repo?) Or do you want to try to add that before I merge?

@igor-dv
Copy link
Member

igor-dv commented Jul 16, 2017

Let's think if it's a blocker ? I prefer to merge this one, and to implement the expansion in separate PR.
@jhurley23 , did you already start working on it ?

@shilman shilman merged commit 65e7341 into storybookjs:release/3.2 Jul 16, 2017
@shilman
Copy link
Member

shilman commented Jul 16, 2017

@jhurley23 @igor-dv OK i've merged this one in. I don't think this is a blocker, but it would be really awesome to get it fixed fully as part of the 3.2 release!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants