Improve search dropdowns #84

tiffon · 2017-09-25T02:40:20Z

Fix Sort operations for a service in TraceSearchForm #79 - Sort services and operations operations (case insensitive)
Fix Change auto-complete boxes to "contains" #31 - Filter options based on contains instead of starts with
Fix The Services dropdown search should be case-insensitive #30 - Filter options based on case insensitive match

- Sort services and operations operations (case insensitive) - Filter options based on contains instead of starts with

coveralls · 2017-09-25T02:44:49Z

Coverage increased (+0.05%) to 56.647% when pulling ae5702f on issue-31-79-dropdowns-need-tlc into 5162309 on master.

yurishkuro · 2017-09-25T04:44:23Z

while at it, I sometimes have an issue where I bring up empty Jaeger UI (e.g. from a fresh all-in-one Docker image), so the Services dropdown is supposed to be empty, but the screen renders with a value from the previous runs on the same URL. I know we store the previously selected service name in local storage, but when we retrieve it we should probably check if the value is actually in the list of currently available services. Feel free to move to a separate issue if needed.

saminzadeh · 2017-09-25T15:24:56Z

src/components/SearchTracePage/SearchDropdownInput.js

+  onSearch(_, searchText) {
+    const { items, maxResults } = this.props;
+    const rxStr = regexpEscape(searchText);
+    const rx = new RegExp(rxStr, 'i');


Small nit: Although this is pretty local in scope, name this to rgx or something else.
rx is common used with RxJS

saminzadeh · 2017-09-25T15:34:58Z

src/utils/regexp-escape.js

+// THE SOFTWARE.
+
+export default function regexpEscape(s) {
+  return s.replace(/[-/\\^$*+?.()|[\]{}]/g, '\\$&');


Maybe a small comment with the use cases this escapes

saminzadeh · 2017-09-25T21:29:48Z

src/reducers/services.js

@@ -35,6 +36,9 @@ function fetchStarted(state) {

 function fetchServicesDone(state, { payload }) {
  const services = payload.data;
+  if (Array.isArray(services)) {
+    services.sort(baseStringComparator);


In my opinion, I think we should use lodash as much as we can on util stuff.

It's well tested, easier to read and immutable.

const sortedServices = _.sortBy(services, s => s) // or if it was an obj { "name": "svc1" } const sortedServices = _.sortBy(services, 'name')

@saminzadeh I would generally go with arr.sort() instead of _.sortBy(arr, value => value). You prefer the lodash util in this case?

The sort util function is using string#localCompare(...) with the option sensitivity. It's ignores accents, case, etc.

I don't think lodash gets us anywhere, here, because it takes a transform function instead of a comparator.

Can you elaborate?

Other than keeping things immutable (redux approach), I think its just about keeping it consistent. Both work, just my 2 cents.

coveralls · 2017-09-26T15:04:40Z

Coverage increased (+0.05%) to 56.647% when pulling 99c2d42 on issue-31-79-dropdowns-need-tlc into 5162309 on master.

tiffon · 2017-09-29T02:07:04Z

@yurishkuro Created ticket #88 to track the issue you mentioned.

* Improve search dropdowns - Sort services and operations operations (case insensitive) - Filter options based on contains instead of starts with * Better var name and added a comment Signed-off-by: vvvprabhakar <vvvprabhakar@gmail.com>

Improve search dropdowns

ae5702f

- Sort services and operations operations (case insensitive) - Filter options based on contains instead of starts with

tiffon assigned saminzadeh Sep 25, 2017

tiffon requested review from jcorbin, whistlinwilly, saminzadeh and a team September 25, 2017 02:40

saminzadeh reviewed Sep 25, 2017

View reviewed changes

Better var name and added a comment

99c2d42

saminzadeh approved these changes Sep 26, 2017

View reviewed changes

tiffon merged commit 61cd543 into master Sep 29, 2017

yurishkuro unassigned saminzadeh Oct 23, 2017

yurishkuro deleted the issue-31-79-dropdowns-need-tlc branch January 29, 2020 15:06

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve search dropdowns #84

Improve search dropdowns #84

tiffon commented Sep 25, 2017 •

edited

Loading

coveralls commented Sep 25, 2017

yurishkuro commented Sep 25, 2017

saminzadeh Sep 25, 2017

saminzadeh Sep 25, 2017

saminzadeh Sep 25, 2017

tiffon Sep 26, 2017

saminzadeh Sep 26, 2017

coveralls commented Sep 26, 2017

tiffon commented Sep 29, 2017

Improve search dropdowns #84

Improve search dropdowns #84

Conversation

tiffon commented Sep 25, 2017 • edited Loading

coveralls commented Sep 25, 2017

yurishkuro commented Sep 25, 2017

saminzadeh Sep 25, 2017

Choose a reason for hiding this comment

saminzadeh Sep 25, 2017

Choose a reason for hiding this comment

saminzadeh Sep 25, 2017

Choose a reason for hiding this comment

tiffon Sep 26, 2017

Choose a reason for hiding this comment

saminzadeh Sep 26, 2017

Choose a reason for hiding this comment

coveralls commented Sep 26, 2017

tiffon commented Sep 29, 2017

tiffon commented Sep 25, 2017 •

edited

Loading