Hierarchy expansion on search #1598

igor-dv · 2017-08-04T20:05:46Z

What I did

Expand all hierarchy when in a filter mode
Highlight found patterns
Added debouncing to the filter

How to test

run cra-kitchen-sink and perform filter

TBD

Add tests !!

…matched parts of the nodes

…-on-search Conflicts: lib/ui/src/modules/ui/components/left_panel/stories_tree/index.js lib/ui/src/modules/ui/components/left_panel/stories_tree/tree_decorators.js

# Conflicts: # lib/ui/src/modules/ui/components/left_panel/stories_tree/tree_decorators.js # lib/ui/src/modules/ui/components/left_panel/stories_tree/tree_style.js

shilman · 2017-08-04T20:11:37Z

dude you're on 🔥 !! looking incredible.

Hypnosphi · 2017-08-04T22:00:04Z

lib/ui/src/modules/ui/components/left_panel/text_filter.js

@@ -1,6 +1,6 @@
 import PropTypes from 'prop-types';
 import React from 'react';
-import debounce from 'lodash.debounce';
+import { debounce } from 'lodash';


What was wrong with the method package?

I didn't manage to mock it...

I think it can be mocked just as identity function: fn => fn

Yeah, that what I did.. But problem looks like in a way of the import.. In the transpiled version
of the import ... from lodash.debounce I am getting a reference to the debounce function. And in transpiled version of named import - the reference is to the lodash object, in this case I managed to replace its method. 🤕

codecov · 2017-08-04T22:11:25Z

Codecov Report

Merging #1598 into master will increase coverage by 0.29%.
The diff coverage is 65.71%.

@@            Coverage Diff             @@
##           master    #1598      +/-   ##
==========================================
+ Coverage   21.05%   21.35%   +0.29%     
==========================================
  Files         244      244              
  Lines        5310     5376      +66     
  Branches      652      660       +8     
==========================================
+ Hits         1118     1148      +30     
- Misses       3690     3726      +36     
  Partials      502      502

Impacted Files	Coverage Δ
...i/components/left_panel/stories_tree/tree_style.js	`25% <ø> (ø)`	⬆️
...b/ui/src/modules/ui/components/left_panel/index.js	`100% <ø> (ø)`	⬆️
...rc/modules/ui/components/left_panel/text_filter.js	`34.24% <42.85%> (+0.91%)`	⬆️
...ponents/left_panel/stories_tree/tree_decorators.js	`32.98% <69.56%> (-0.35%)`	⬇️
...les/ui/components/left_panel/stories_tree/index.js	`98.52% <94.73%> (-1.48%)`	⬇️
.../src/manager/containers/CommentsPanel/dataStore.js	`34.78% <0%> (ø)`	⬆️
addons/notes/src/react.js	`0% <0%> (ø)`	⬆️
lib/ui/src/modules/shortcuts/actions/shortcuts.js	`6.25% <0%> (ø)`	⬆️
... and 24 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 38ff4cb...24a4140. Read the comment docs.

ndelangen · 2017-08-04T23:16:46Z

🔥

Hypnosphi · 2017-08-05T00:10:54Z

lib/ui/src/modules/ui/components/left_panel/stories_tree/tree_decorators.js

+      return name;
+    }
+
+    const filterRegex = new RegExp(`(${storyFilter})`, 'i');


Looks like this will break with special characters, plus it's not how the stories are actually filtered. I'd suggest to use Fuse.js with { includeMatches: true } in libs/filters.js and store the matches information in some dedicated field on the filtered stories

igor-dv · 2017-08-05T12:20:19Z

Chatted with @Hypnosphi in slack.

Our current fuzzysearch lib is a bit limiting, since the only result it gives is whether the string matches to the query.

Already mentioned Fuse.js lib is very promising and can help us improve both filtering and highlighting features (built-in matches indices in the result). However they have a PR that is vital for us. So we can make a fork (like with the react-treabeard).

Other alternative might be this small lib - https://github.com/mattyork/fuzzy/blob/master/lib/fuzzy.js.
It has no built in matches feature, but can be easily used as a source of truth for both - filtering and highlighting.

In other words I think this change is bigger than just PR to master..

What do you guys think ?

Hypnosphi · 2017-08-05T12:26:21Z

However they have a PR that is vital for us. So we can make a fork (like with the react-treabeard).

We can make a fork and send a new PR with tests =)

It has no built in matches feature, but can be easily used as a source of truth for both - filtering and highlighting.

How exactly would you use it for highlighting?

shilman · 2017-08-05T15:00:52Z

@igor-dv I think the current behavior is perfect for most real-world component libraries, which never grow particularly big/complex. You're always welcome to make it better, but if it was me I would just ship it as is.

shilman

LATM! (A = awesome!)

Hypnosphi · 2017-08-05T15:47:05Z

@shilman

Please type \ in the filter and look into console
Try to skip a letter in a search request. Stories are filtered, but not highlighted. What does it have to do with library size?

It looks really awesome to me, too, but those two issues should be addressed (at least the first one).

# Conflicts: # lib/ui/src/modules/ui/components/left_panel/index.js

igor-dv · 2017-08-05T18:33:18Z

I've fixed the special characters issue.

@shilman regarding the highlighting, here is an example when it breaks..

I think if there are no strong objections we can merge this one (after I'll add few tests, probably tomorrow) And add the Fuse.js as a separate PR (and maybe as part of the next release).

Hypnosphi · 2017-08-05T18:56:59Z

Great! The first issue is gone, the second one can be solved in a scope of another PR

igor-dv added 5 commits August 4, 2017 18:52

Expand stories by default when the search filter applied + highlight …

834c1cb

…matched parts of the nodes

Merge branch 'master' into hierarchy-expansion-on-search

dc19cbb

Merge remote-tracking branch 'origin/master' into hierarchy-expansion…

392e2e1

…-on-search Conflicts: lib/ui/src/modules/ui/components/left_panel/stories_tree/index.js lib/ui/src/modules/ui/components/left_panel/stories_tree/tree_decorators.js

Remove comments

6cbe9de

Merge branch 'master' into hierarchy-expansion-on-search

0eb1984

# Conflicts: # lib/ui/src/modules/ui/components/left_panel/stories_tree/tree_decorators.js # lib/ui/src/modules/ui/components/left_panel/stories_tree/tree_style.js

igor-dv added ci: do not merge feature request in progress ui labels Aug 4, 2017

Hypnosphi self-requested a review August 4, 2017 20:34

Fix filter tests

1dab405

Hypnosphi reviewed Aug 4, 2017

View reviewed changes

Cleanups

5cb61d8

igor-dv added 2 commits August 5, 2017 01:47

Rename for readability

e280f3a

Mock 'lodash.debounce' import

0d32851

Hypnosphi requested changes Aug 5, 2017

View reviewed changes

shilman approved these changes Aug 5, 2017

View reviewed changes

igor-dv added 2 commits August 5, 2017 20:27

Merge branch 'master' into hierarchy-expansion-on-search

8e0e6d6

# Conflicts: # lib/ui/src/modules/ui/components/left_panel/index.js

Remove special characters from stories filter

435bf98

Escape instead of replacing

ff2e98a

Hypnosphi approved these changes Aug 5, 2017

View reviewed changes

Add test for highlighting

24a4140

igor-dv removed ci: do not merge in progress labels Aug 6, 2017

Hypnosphi merged commit ba9d520 into master Aug 6, 2017

Hypnosphi deleted the hierarchy-expansion-on-search branch August 6, 2017 10:23

igor-dv mentioned this pull request Aug 11, 2017

Improve search and highlighting #1640

Merged

Hypnosphi mentioned this pull request Aug 20, 2017

Hotfix for issue with stories panel #1689

Merged

igor-dv mentioned this pull request Aug 20, 2017

[New] Improve search and highlighting #1693

Merged

dependencies bot mentioned this pull request Jan 11, 2018

Update 25 dependencies from npm #2724

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hierarchy expansion on search #1598

Hierarchy expansion on search #1598

igor-dv commented Aug 4, 2017

shilman commented Aug 4, 2017

Hypnosphi Aug 4, 2017 •

edited

Loading

igor-dv Aug 4, 2017

Hypnosphi Aug 4, 2017

igor-dv Aug 4, 2017

codecov bot commented Aug 4, 2017 •

edited

Loading

ndelangen commented Aug 4, 2017

Hypnosphi Aug 5, 2017 •

edited

Loading

igor-dv commented Aug 5, 2017

Hypnosphi commented Aug 5, 2017

shilman commented Aug 5, 2017

shilman left a comment

Hypnosphi commented Aug 5, 2017 •

edited

Loading

igor-dv commented Aug 5, 2017

Hypnosphi commented Aug 5, 2017

Hierarchy expansion on search #1598

Hierarchy expansion on search #1598

Conversation

igor-dv commented Aug 4, 2017

What I did

How to test

TBD

shilman commented Aug 4, 2017

Hypnosphi Aug 4, 2017 • edited Loading

Choose a reason for hiding this comment

igor-dv Aug 4, 2017

Choose a reason for hiding this comment

Hypnosphi Aug 4, 2017

Choose a reason for hiding this comment

igor-dv Aug 4, 2017

Choose a reason for hiding this comment

codecov bot commented Aug 4, 2017 • edited Loading

Codecov Report

ndelangen commented Aug 4, 2017

Hypnosphi Aug 5, 2017 • edited Loading

Choose a reason for hiding this comment

igor-dv commented Aug 5, 2017

Hypnosphi commented Aug 5, 2017

shilman commented Aug 5, 2017

shilman left a comment

Choose a reason for hiding this comment

Hypnosphi commented Aug 5, 2017 • edited Loading

igor-dv commented Aug 5, 2017

Hypnosphi commented Aug 5, 2017

Hypnosphi Aug 4, 2017 •

edited

Loading

codecov bot commented Aug 4, 2017 •

edited

Loading

Hypnosphi Aug 5, 2017 •

edited

Loading

Hypnosphi commented Aug 5, 2017 •

edited

Loading