Skip to content
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.

Sign up for GitHub

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

Merged
merged 15 commits into from
Jan 29, 2024
Merged

Add debounce to query update #88

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
af74051
fix issue with toggle
TalMalka123 Jan 25, 2024
42d12c0
wrap debounced func in a useCallback
TalMalka123 Jan 25, 2024
655f2a0
value provided to Autocomplete is invalid
TalMalka123 Jan 25, 2024
e0e4910
fmt
TalMalka123 Jan 25, 2024
e360fb7
bump version
TalMalka123 Jan 25, 2024
de48d35
fix use effect dependencies
TalMalka123 Jan 25, 2024
5e3b379
fix toggle issue in a different way
TalMalka123 Jan 25, 2024
a88dde3
run fmt
TalMalka123 Jan 25, 2024
54a0347
simplify context
TalMalka123 Jan 25, 2024
df5300c
simplify context
TalMalka123 Jan 25, 2024
923dab6
update var name
TalMalka123 Jan 25, 2024
7699737
fmt
TalMalka123 Jan 25, 2024
6c589f2
fix pr comments
TalMalka123 Jan 29, 2024
cafc55f
Merge remote-tracking branch 'origin/main' into add-debounce-to-query…
TalMalka123 Jan 29, 2024
fd5399b
bump verision
TalMalka123 Jan 29, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion src/components/dagshub/data-engine/CSVViewer/CSVViewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ export function CSVViewer({ headers, values }: { headers: string[]; values: stri

const autoSizeStrategy: SizeColumnsToFitGridStrategy = {
type: 'fitGridWidth',
defaultMinWidth: 100,
defaultMinWidth: 100
};

return (
Expand Down
6 changes: 5 additions & 1 deletion src/components/dagshub/data-engine/queryBuilder/ConditionTextField.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import StyledTextField from '../metadataKeyValue/StyledTextField';
import '../metadataKeyValue/style.scss';
import { ThemeProvider, Typography } from '@mui/material';
import theme from '../../../../theme';
import {useDebounce} from "react-use";

export function ConditionTextField({
disabled,
Expand Down Expand Up @@ -67,6 +68,10 @@ export function ConditionTextField({
}
};

useDebounce(() => {
onChange(currentValue);
},200, [currentValue]);

return (
<ThemeProvider theme={theme}>
<Box
Expand Down Expand Up @@ -137,7 +142,6 @@ export function ConditionTextField({
}}
onChange={(e: any) => {
setCurrentValue(e.target.value);
onChange(e.target.value);
}}
onKeyDown={handleKeyDown}
value={currentValue}
Expand Down
3 changes: 0 additions & 3 deletions src/components/dagshub/data-engine/queryBuilder/QueryBuilder.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,14 +11,12 @@ import { MetadataType } from '../metadataKeyValue/MetadataKeyValueList';
export function QueryBuilder({
queryInput,
metadataFields,
forceCompoundMode = false,
onChange,
validateValueByType,
showConditionSummary = false,
}: {
queryInput: QueryInput;
metadataFields: MetadataFieldProps[]; // need to take into consideration the select and the alias
forceCompoundMode?: boolean;
onChange: (query: QueryInput) => void;
validateValueByType: (valueType: MetadataType, value: string, comparator: Comparator) => boolean;
showConditionSummary?: boolean;
Expand All @@ -27,7 +25,6 @@ export function QueryBuilder({
<QueryBuilderProvider
queryInput={queryInput}
metadataFields={metadataFields}
forceCompoundMode={forceCompoundMode}
validateValueByType={validateValueByType}
onChange={onChange}
>
Expand Down
50 changes: 21 additions & 29 deletions src/components/dagshub/data-engine/queryBuilder/QueryBuilderContext.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import React, {
useState,
} from 'react';
import _ from 'lodash';
import { root } from 'postcss';

type MetadataType = 'BOOLEAN' | 'INTEGER' | 'FLOAT' | 'STRING' | 'BLOB';

Expand Down Expand Up @@ -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 }[];
Expand All @@ -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;
}) => {
Expand Down Expand Up @@ -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);
Copy link
Contributor

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

Copy link
Contributor Author

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


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) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setIsSimpleMode((isSimpleMode) => {
setIsSimpleMode(!isSimpleMode)

I also didnt get the correlation between the isDisplayinSimpleMode / isSimpleMode. it cant be the same value ?

Copy link
Contributor Author

@TalMalka123 TalMalka123 Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

@simonlsk simonlsk Jan 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

The 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.
I personally prefer erring on the safe side an always use previous state.

if (isSimpleMode) {
return false;
} else {
return isDisplayableInSimpleMode;
}
});
}

useEffect(() => {
setMetadataFieldsList(metadataFields);
}, [metadataFields]);
setIsSimpleMode((isSimpleMode) => {
Copy link
Contributor

@elad-n elad-n Jan 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
setIsSimpleMode((isSimpleMode) => {
const updatedIsSimpleMode = isSimpleMode ? false : isDisplayableInSimpleMode)
setIsSimpleMode(updatedIsSimpleMode)

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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) => {
Expand Down Expand Up @@ -419,10 +412,9 @@ export const QueryBuilderProvider = ({
<QueryBuilderContext.Provider
value={{
isSimpleMode,
setIsSimpleMode,
rootCondition,
setRootCondition,
metadataFieldsList,
metadataFields,
generateUniqueId,
addUniqueIds,
getOperatorsByMetadataType,
Expand Down
13 changes: 6 additions & 7 deletions src/components/dagshub/data-engine/queryBuilder/SimpleCondition.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ export function SimpleCondition({
const {
isSimpleMode,
validateValueByType,
metadataFieldsList,
metadataFields,
getOperatorsByMetadataType,
checkIfOperatorRequiresValueField,
} = useQueryBuilderContext();
Expand Down Expand Up @@ -68,7 +68,7 @@ export function SimpleCondition({
const [isEmpty, setIsEmpty] = useState<boolean>(checkIfConditionIsUncompleted());

function getSelectedMetadataKey() {
return metadataFieldsList?.find((field) => field.name === condition.filter?.key);
return metadataFields?.find((field) => field.name === condition.filter?.key);
}

const [selectedMetadataKey, setSelectedMetadataKey] = useState<MetadataFieldProps | undefined>(
Expand All @@ -89,7 +89,7 @@ export function SimpleCondition({

useEffect(() => {
setSelectedMetadataKey(getSelectedMetadataKey());
}, [metadataFieldsList, condition.filter?.key]);
}, [metadataFields, condition.filter?.key]);

useEffect(() => {
setSelectedOperator(getSelectedComparator());
Expand Down Expand Up @@ -208,12 +208,11 @@ export function SimpleCondition({
filter: {
...condition.filter,
key: value?.id,
valueType: metadataFieldsList.find((field) => field.name === value?.label)
?.valueType,
},
valueType: metadataFields.find((field) => field.name === value?.label)?.valueType,
}
});
}}
options={metadataFieldsList?.map((field) => ({ id: field.name, label: field.name }))}
options={metadataFields?.map((field) => ({ id: field.name, label: field.name }))}
/>
<ConditionDropdown
isReadOnly={true}
Expand Down
3 changes: 3 additions & 0 deletions src/components/elements/dropdownV2/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,6 +173,9 @@ export function DropdownV2({
</Typography>
{/*This is a hidden div that is used to calculate the width of the input field*/}
<Autocomplete
isOptionEqualToValue={(option: RadioButtonItemProps, value: RadioButtonItemProps) =>
option.id === value.id && option.label === value.label
}
noOptionsText={
<Typography
sx={{
Expand Down
Loading