Importing list of values to String filter menu #419

adrianmroz · 2019-05-10T09:33:38Z

No description provided.

bigsergey · 2019-05-10T09:42:17Z

src/client/components/selectable-string-filter-menu/rows-list.tsx

+
+const Row: React.SFC<RowProps> = props => {
+  const { value, selected, checkboxStyle, highlight, onRowSelect } = props;
+  const segmentValueStr = String(value);


segmentValueString

bigsergey · 2019-05-10T09:43:53Z

src/client/components/selectable-string-filter-menu/rows-list.tsx

+  const searchTextLower = searchText.toLowerCase();
+  return rows.filter(d => {
+    return String(d).toLowerCase().indexOf(searchTextLower) !== -1;
+  });


return rows.filter(d => String(d).toLowerCase().indexOf(searchTextLower) !== -1);

bigsergey · 2019-05-10T09:44:36Z

src/client/components/selectable-string-filter-menu/rows-list.tsx

+  onRowSelect: Binary<unknown, React.MouseEvent<HTMLDivElement>, void>;
+}
+
+const Row: React.SFC<RowProps> = props => {


move component to separate file

bigsergey · 2019-05-10T09:45:07Z

src/client/components/selectable-string-filter-menu/rows-list.tsx

+  const rows = dataset.data.slice(0, limit).map(d => d[dimension.name] as string);
+  const matchingRows = filterRows(rows, searchText);
+  if (searchText && matchingRows.length === 0) {
+    return <div className="message">{'No results for "' + searchText + '"'}</div>;


template string?

linter got confused because of JSX

bigsergey · 2019-05-10T09:46:50Z

src/client/components/selectable-string-filter-menu/rows-list.tsx

+      onRowSelect={onRowSelect}
+      selected={selectedValues && selectedValues.contains(value)}
+      checkboxStyle={checkboxStyle}
+      highlight={searchText} />)}


{matchingRows.map(value => ( <Row key={String(value)} value={value} onRowSelect={onRowSelect} selected={selectedValues && selectedValues.contains(value)} checkboxStyle={checkboxStyle} highlight={searchText} /> ))}

bigsergey · 2019-05-10T09:48:36Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx

-      dataset: null,
-      error: null,
-      fetchQueued: false,
+      dataset: loading,


Maybe it will be better to move loading to another variable? isDatasetLoaded?

bigsergey · 2019-05-10T09:49:02Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx

+        // null is here when we get out of order request, so we just ignore it
+        if (!dataset) return;
+        this.setState({ dataset });
+      });


I don't need catch there, I've guarantee that this promising only resolves, never rejects. But I can't enforce this in types and/or in convention.

bigsergey · 2019-05-10T09:50:03Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx

-      .sort($("MEASURE"), SortExpression.DESCENDING)
-      .limit(TOP_N + 1);
+  private queryFilter = (essence: Essence, timekeeper: Timekeeper, dimension: Dimension, searchText: string): Promise<DatasetLoad> => {
+    const query = constructQuery(essence, timekeeper, dimension, searchText, TOP_N + 1);


too many parameters, object is better option for constructQuery function

bigsergey · 2019-05-10T09:50:35Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx

-          });
+          if (this.lastSearchText !== searchText) return null;
+          reportError(error);
+          return error(error);


move it to catch

looks like a matter of preference.

bigsergey · 2019-05-10T09:51:28Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx

  }

  componentWillUnmount() {
-    this.mounted = false;
+    this.debouncedQueryFilter.cancel();


erykpiast

Good job 👍

erykpiast · 2019-05-17T06:58:28Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx

-      .apply("MEASURE", measureExpression)
-      .sort($("MEASURE"), SortExpression.DESCENDING)
-      .limit(TOP_N + 1);
+  private queryFilter = (essence: Essence, timekeeper: Timekeeper, dimension: Dimension, searchText: string): Promise<DatasetLoad> => {


essence: Essence, timekeeper: Timekeeper, dimension: Dimension, searchText: string <- you repeat this signature at least three times, name it.

you already have it named 🤔 SelectableStringFilterMenuProps

erykpiast · 2019-05-17T06:59:07Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx

-          });
+          if (this.lastSearchText !== searchText) return null;
+          reportError(error);
+          return error(error);


looks like a matter of preference.

erykpiast · 2019-05-17T07:43:38Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx

@@ -207,7 +160,7 @@ export class SelectableStringFilterMenu extends React.Component<SelectableString
    return onClauseChange(clause);
  }

-  onValueClick(value: any, e: MouseEvent) {
+  onValueClick = (value: string, e: React.MouseEvent<HTMLDivElement>) => {
    const { selectedValues, colors: oldColors } = this.state;
    const colors = oldColors && oldColors.toggle(value);
    if (e.altKey || e.ctrlKey || e.metaKey) {


what if this handler is called with withModifier: bool instead of the whole event? wouldn't it be a better separation of concerns?

erykpiast · 2019-05-17T07:48:23Z

src/client/components/selectable-string-filter-menu/import-form.tsx

+
+  render() {
+    const { value } = this.state;
+    const {} = this.props;


erykpiast · 2019-05-17T07:49:34Z

src/client/components/selectable-string-filter-menu/selectable-string-filter-menu.tsx


-  private loadRows(essence: Essence, timekeeper: Timekeeper, dimension: Dimension, searchText: string) {
+  private loadRows(props: QueryProps) {


oh, you just did it!

erykpiast · 2019-05-17T07:53:10Z

src/client/components/preview-string-filter-menu/preview-list.tsx

+  }
+}
+
+function filterValues(list: Array<unknown>, filterMode: PreviewFilterMode, searchText: string): Array<unknown> {


unknown -> T?

Yup! It will substitute T to unknown but it will be the same unknown on the output!

erykpiast · 2019-05-17T08:04:50Z

src/client/components/filter-menu/string-filter-menu/string-filter-menu.tsx

+    const { dimension, clicker, essence, timekeeper, onClose } = this.props;
+    const { filterMode } = this.state;
+    const onClauseChange = this.updateFilter;
+    const props = { dimension, clicker, essence, timekeeper, onClose, onClauseChange };


why don't you add filterMode right here? then you could switch used component only.

TSX typing issues. If I add this there, props will have filterMode: FilterMode, and switch wouldn't narrow it's type inside object so types on component's won't match. If I narrow type via switch and then add to object, it has correct type.

mkuthan · 2019-05-27T06:03:50Z

src/client/components/preview-string-filter-menu/preview-list.scss

@@ -0,0 +1,63 @@
+/*
+ * Copyright 2015-2016 Imply Data, Inc.
+ * Copyright 2017-2018 Allegro.pl


Allegro only

adrianmroz changed the title ~~Initial component refactor~~ Importing list of values to String filter menu May 10, 2019

bigsergey reviewed May 10, 2019

View reviewed changes

mkuthan added this to the 1.16 milestone May 10, 2019

adrianmroz marked this pull request as ready for review May 13, 2019 13:45

erykpiast approved these changes May 17, 2019

View reviewed changes

adrianmroz-allegro added 12 commits May 20, 2019 11:57

Initial component refactor

cee9231

Code style fixes

2902610

overflow long string values in filter menu

7b19a3c

Don't close menu on enter when in import mode

17ddbdd

Code style fixes

dc7acbc

Refactor PreviewStringFilter

5095492

Show "Matching values" label only when search text is present

b9ef6ca

Move search box to lower components

05d0701

Move css around

e1cb722

Move import-form to new component paste-form

7127363

Better types

c96de00

Handle error that shouldn't be thrown.

d5fc0ff

adrianmroz force-pushed the feature/import-values-to-string-filter branch from 008ca83 to d5fc0ff Compare May 20, 2019 10:05

search text is local state for preview string filter

e48d7c1

mkuthan reviewed May 27, 2019

View reviewed changes

adrianmroz-allegro and others added 2 commits May 27, 2019 09:41

Remove unnecessary copyright

b7908f0

Merge branch 'master' into feature/import-values-to-string-filter

60c9b6c

adrianmroz merged commit a5a7a25 into master May 28, 2019

adrianmroz deleted the feature/import-values-to-string-filter branch May 28, 2019 11:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Importing list of values to String filter menu #419

Importing list of values to String filter menu #419

adrianmroz commented May 10, 2019

bigsergey May 10, 2019

bigsergey May 10, 2019

bigsergey May 10, 2019

bigsergey May 10, 2019

adrianmroz May 10, 2019

bigsergey May 10, 2019

bigsergey May 10, 2019

bigsergey May 10, 2019

adrianmroz May 17, 2019

bigsergey May 10, 2019

bigsergey May 10, 2019

erykpiast May 17, 2019

bigsergey May 10, 2019

erykpiast left a comment

erykpiast May 17, 2019

erykpiast May 17, 2019

erykpiast May 17, 2019

erykpiast May 17, 2019

erykpiast May 17, 2019

erykpiast May 17, 2019

erykpiast May 17, 2019

adrianmroz May 17, 2019

erykpiast May 17, 2019

adrianmroz May 17, 2019

mkuthan May 27, 2019


		private loadRows(essence: Essence, timekeeper: Timekeeper, dimension: Dimension, searchText: string) {
		private loadRows(props: QueryProps) {

Importing list of values to String filter menu #419

Importing list of values to String filter menu #419

Conversation

adrianmroz commented May 10, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

erykpiast left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment