-
Notifications
You must be signed in to change notification settings - Fork 0
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
Add debounce to query update #88
Changes from all commits
af74051
42d12c0
655f2a0
e0e4910
e360fb7
de48d35
5e3b379
a88dde3
54a0347
df5300c
923dab6
7699737
6c589f2
cafc55f
fd5399b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change | ||||||
---|---|---|---|---|---|---|---|---|
|
@@ -7,7 +7,6 @@ import React, { | |||||||
useState, | ||||||||
} from 'react'; | ||||||||
import _ from 'lodash'; | ||||||||
import { root } from 'postcss'; | ||||||||
|
||||||||
type MetadataType = 'BOOLEAN' | 'INTEGER' | 'FLOAT' | 'STRING' | 'BLOB'; | ||||||||
|
||||||||
|
@@ -100,10 +99,9 @@ export interface QueryInput { | |||||||
|
||||||||
interface QueryBuilderContextInterface { | ||||||||
isSimpleMode: boolean; | ||||||||
setIsSimpleMode: (isSimpleMode: boolean) => void; | ||||||||
rootCondition: AndOrMetadataInput; | ||||||||
setRootCondition: (rootCondition: AndOrMetadataInput) => void; | ||||||||
metadataFieldsList: MetadataFieldProps[]; | ||||||||
metadataFields: MetadataFieldProps[]; | ||||||||
generateUniqueId: () => string; | ||||||||
addUniqueIds: (input: AndOrMetadataInput) => AndOrMetadataInput; | ||||||||
getOperatorsByMetadataType: (type: MetadataType) => { label: string; id: Comparator }[]; | ||||||||
|
@@ -129,14 +127,12 @@ export const QueryBuilderProvider = ({ | |||||||
children, | ||||||||
queryInput, | ||||||||
metadataFields, | ||||||||
forceCompoundMode = false, | ||||||||
validateValueByType, | ||||||||
onChange, | ||||||||
}: { | ||||||||
children: ReactNode; | ||||||||
queryInput: QueryInput; | ||||||||
metadataFields: MetadataFieldProps[]; // need to take into consideration the select and the alias | ||||||||
forceCompoundMode?: boolean; | ||||||||
validateValueByType: (valueType: MetadataType, value: string, comparator: Comparator) => boolean; | ||||||||
onChange: (query: QueryInput) => void; | ||||||||
}) => { | ||||||||
|
@@ -170,48 +166,45 @@ export const QueryBuilderProvider = ({ | |||||||
return true; | ||||||||
}; | ||||||||
|
||||||||
const checkIfSimpleMode = useCallback( | ||||||||
(query: AndOrMetadataInput | undefined) => { | ||||||||
return !forceCompoundMode && checkIfConditionIsDisplayableInSimpleMode(query); | ||||||||
}, | ||||||||
[forceCompoundMode] | ||||||||
); | ||||||||
|
||||||||
const [rootCondition, setRootCondition] = useState<AndOrMetadataInput>(() => getInitialQuery()); | ||||||||
const [isSimpleMode, setIsSimpleMode] = useState<boolean>(() => | ||||||||
checkIfSimpleMode(queryInput.query) | ||||||||
); | ||||||||
const [metadataFieldsList, setMetadataFieldsList] = | ||||||||
useState<MetadataFieldProps[]>(metadataFields); | ||||||||
const [isDisplayableInSimpleMode, setIsDisplayableInSimpleMode] = useState<boolean>( | ||||||||
const [rootCondition, setRootCondition] = useState<AndOrMetadataInput>(getInitialQuery); | ||||||||
const [isDisplayableInSimpleMode, setIsDisplayableInSimpleMode] = useState<boolean>(() => | ||||||||
checkIfConditionIsDisplayableInSimpleMode(queryInput.query) | ||||||||
); | ||||||||
const [isSimpleMode, setIsSimpleMode] = useState<boolean>(isDisplayableInSimpleMode); | ||||||||
|
||||||||
useEffect(() => { | ||||||||
if ( | ||||||||
JSON.stringify(removeIdFields(getInitialQuery())) !== | ||||||||
JSON.stringify(removeIdFields(rootCondition)) | ||||||||
) { | ||||||||
setRootCondition(getInitialQuery); | ||||||||
setIsDisplayableInSimpleMode(checkIfConditionIsDisplayableInSimpleMode(queryInput.query)); | ||||||||
} | ||||||||
setIsDisplayableInSimpleMode(checkIfConditionIsDisplayableInSimpleMode(queryInput.query)); | ||||||||
}, [queryInput.query]); | ||||||||
|
||||||||
useEffect(() => { | ||||||||
setIsDisplayableInSimpleMode(checkIfConditionIsDisplayableInSimpleMode(rootCondition)); | ||||||||
}, [rootCondition]); | ||||||||
|
||||||||
useEffect(() => { | ||||||||
setIsSimpleMode(checkIfSimpleMode(queryInput.query)); | ||||||||
}, [forceCompoundMode, queryInput.query]); | ||||||||
|
||||||||
function onToggleQueryMode() { | ||||||||
setIsSimpleMode(!isSimpleMode); | ||||||||
setIsSimpleMode((isSimpleMode) => { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
I also didnt get the correlation between the isDisplayinSimpleMode / isSimpleMode. it cant be the same value ? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @elad-n - Regarding your suggested change, @simonlsk told me that when I want to change the state, I should not use the variable from the state definition (ie. setBool(!bool)), but use the prev from the function instead, so there won't be race conditioning @elad-n And regarding your question - isDisplayinSimpleMode just holds whether it is possible to switch the toggle to compound mode, but it doesn't say anything about the current query builder mode state, unlike isSimpleMode, which is the state itself. So they are not the same. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In that case I think both will work correctly, but following the official docs using previous state is preferable if the set function can be called multiple times. |
||||||||
if (isSimpleMode) { | ||||||||
return false; | ||||||||
} else { | ||||||||
return isDisplayableInSimpleMode; | ||||||||
} | ||||||||
}); | ||||||||
} | ||||||||
|
||||||||
useEffect(() => { | ||||||||
setMetadataFieldsList(metadataFields); | ||||||||
}, [metadataFields]); | ||||||||
setIsSimpleMode((isSimpleMode) => { | ||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess its the same comment like before: #88 (comment) |
||||||||
if (isSimpleMode) { | ||||||||
return isDisplayableInSimpleMode; | ||||||||
} else { | ||||||||
return false; | ||||||||
} | ||||||||
}); | ||||||||
}, [isDisplayableInSimpleMode]); | ||||||||
|
||||||||
//This function is used to remove the root and wrapper, if it was added for ui purposes and not needed anymore | ||||||||
const removeRootAndBlockIfWasAddedAndNotNeeded = (condition: AndOrMetadataInput | null) => { | ||||||||
|
@@ -419,10 +412,9 @@ export const QueryBuilderProvider = ({ | |||||||
<QueryBuilderContext.Provider | ||||||||
value={{ | ||||||||
isSimpleMode, | ||||||||
setIsSimpleMode, | ||||||||
rootCondition, | ||||||||
setRootCondition, | ||||||||
metadataFieldsList, | ||||||||
metadataFields, | ||||||||
generateUniqueId, | ||||||||
addUniqueIds, | ||||||||
getOperatorsByMetadataType, | ||||||||
|
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.
useState hooks should be in the top of the hook, heres why:
https://legacy.reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
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'm not sure I understand. My use-state hooks are at the top. Before them there are just functions that the state uses, so they must be defined before the state itself, for them to be recognized.
According to the article, it doesn't seem to break any rule cause there is no prior return statement within the component itself (unless I miss something), and the hooks are not within any function/condition.
@elad-n Let me know if I miss something, and what I need to do