-
Notifications
You must be signed in to change notification settings - Fork 170
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
Importing list of values to String filter menu #419
Conversation
|
||
const Row: React.SFC<RowProps> = props => { | ||
const { value, selected, checkboxStyle, highlight, onRowSelect } = props; | ||
const segmentValueStr = String(value); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
segmentValueString
const searchTextLower = searchText.toLowerCase(); | ||
return rows.filter(d => { | ||
return String(d).toLowerCase().indexOf(searchTextLower) !== -1; | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return rows.filter(d => String(d).toLowerCase().indexOf(searchTextLower) !== -1);
onRowSelect: Binary<unknown, React.MouseEvent<HTMLDivElement>, void>; | ||
} | ||
|
||
const Row: React.SFC<RowProps> = props => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move component to separate file
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>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
template string?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
linter got confused because of JSX
onRowSelect={onRowSelect} | ||
selected={selectedValues && selectedValues.contains(value)} | ||
checkboxStyle={checkboxStyle} | ||
highlight={searchText} />)} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
{matchingRows.map(value => (
<Row
key={String(value)}
value={value}
onRowSelect={onRowSelect}
selected={selectedValues && selectedValues.contains(value)}
checkboxStyle={checkboxStyle}
highlight={searchText}
/>
))}
dataset: null, | ||
error: null, | ||
fetchQueued: false, | ||
dataset: loading, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it will be better to move loading to another variable? isDatasetLoaded?
// null is here when we get out of order request, so we just ignore it | ||
if (!dataset) return; | ||
this.setState({ dataset }); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
.catch
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
.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); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
too many parameters, object is better option for constructQuery function
}); | ||
if (this.lastSearchText !== searchText) return null; | ||
reportError(error); | ||
return error(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
move it to catch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a matter of preference.
} | ||
|
||
componentWillUnmount() { | ||
this.mounted = false; | ||
this.debouncedQueryFilter.cancel(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job 👍
.apply("MEASURE", measureExpression) | ||
.sort($("MEASURE"), SortExpression.DESCENDING) | ||
.limit(TOP_N + 1); | ||
private queryFilter = (essence: Essence, timekeeper: Timekeeper, dimension: Dimension, searchText: string): Promise<DatasetLoad> => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
essence: Essence, timekeeper: Timekeeper, dimension: Dimension, searchText: string
<- you repeat this signature at least three times, name it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you already have it named 🤔 SelectableStringFilterMenuProps
}); | ||
if (this.lastSearchText !== searchText) return null; | ||
reportError(error); | ||
return error(error); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks like a matter of preference.
@@ -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) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what if this handler is called with withModifier: bool
instead of the whole event? wouldn't it be a better separation of concerns?
|
||
render() { | ||
const { value } = this.state; | ||
const {} = this.props; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
WTF
|
||
private loadRows(essence: Essence, timekeeper: Timekeeper, dimension: Dimension, searchText: string) { | ||
private loadRows(props: QueryProps) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh, you just did it!
} | ||
} | ||
|
||
function filterValues(list: Array<unknown>, filterMode: PreviewFilterMode, searchText: string): Array<unknown> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
unknown
-> T
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! It will substitute T
to unknown
but it will be the same unknown
on the output!
const { dimension, clicker, essence, timekeeper, onClose } = this.props; | ||
const { filterMode } = this.state; | ||
const onClauseChange = this.updateFilter; | ||
const props = { dimension, clicker, essence, timekeeper, onClose, onClauseChange }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why don't you add filterMode
right here? then you could switch used component only.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
008ca83
to
d5fc0ff
Compare
@@ -0,0 +1,63 @@ | |||
/* | |||
* Copyright 2015-2016 Imply Data, Inc. | |||
* Copyright 2017-2018 Allegro.pl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Allegro only
No description provided.