-
Notifications
You must be signed in to change notification settings - Fork 61
[ENHANCEMENT] TimeSeriesChart: add query name support for query settings #585
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
base: main
Are you sure you want to change the base?
Changes from all commits
2f19a36
71e2bb7
59cbfe1
7b238ae
eb2f80f
2aa0ea3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,7 +31,7 @@ import DeleteIcon from 'mdi-material-ui/DeleteOutline'; | |
| import AddIcon from 'mdi-material-ui/Plus'; | ||
| import CloseIcon from 'mdi-material-ui/Close'; | ||
| import { produce } from 'immer'; | ||
| import { useDataQueriesContext } from '@perses-dev/plugin-system'; | ||
| import { generateQueryNames, useDataQueriesContext, defaultQueryName } from '@perses-dev/plugin-system'; | ||
| import { | ||
| DEFAULT_AREA_OPACITY, | ||
| LINE_STYLE_CONFIG, | ||
|
|
@@ -42,35 +42,68 @@ import { | |
| } from './time-series-chart-model'; | ||
|
|
||
| const DEFAULT_COLOR_VALUE = '#555'; | ||
| const NO_INDEX_AVAILABLE = -1; // invalid array index value used to represent the fact that no query index is available | ||
| const NO_QUERY_AVAILABLE = '-1'; // sentinel value used to represent the fact that no query is available | ||
|
|
||
| /** | ||
| * Resolves the effective query name from a settings entry. | ||
| * Falls back to the deprecated `queryIndex` field when `queryName` is absent. | ||
| */ | ||
| function resolveQueryName(querySettings: QuerySettingsOptions, queryNames: string[]): string { | ||
| if (querySettings.queryName !== undefined) { | ||
| return querySettings.queryName; | ||
| } | ||
| if (querySettings.queryIndex !== undefined) { | ||
| return queryNames[querySettings.queryIndex] ?? defaultQueryName(querySettings.queryIndex); | ||
| } | ||
| return ''; | ||
| } | ||
|
|
||
| /** | ||
| * Normalizes a settings entry so it always persists via `queryName`. | ||
| * Converts the deprecated `queryIndex` to `queryName` and removes the old field. | ||
| */ | ||
| function normalizeToQueryName(querySettings: QuerySettingsOptions, queryNames: string[]): QuerySettingsOptions { | ||
| if (querySettings.queryIndex === undefined) { | ||
| return querySettings; | ||
| } | ||
|
Comment on lines
+66
to
+68
Contributor
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. It seems that the idea behind this condition was that, if /* ALL FIELDS ARE OPTIONAL */
export interface QuerySettingsOptions {
queryName?: string;
/**
* @deprecated Use `queryName` instead.
*/
queryIndex?: number;
colorMode?: 'fixed' | 'fixed-single';
colorValue?: string;
lineStyle?: LineStyleType;
areaOpacity?: number;
format?: FormatOptions;
}So, maybe? if (querySettings.queryIndex === undefined && querySettings.queryName!==undefined) |
||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| const { queryIndex, ...rest } = querySettings; | ||
| return { ...rest, queryName: rest.queryName ?? queryNames[queryIndex] ?? defaultQueryName(queryIndex) }; | ||
| } | ||
|
|
||
| export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): ReactElement { | ||
| const { onChange, value } = props; | ||
| const querySettingsList = value.querySettings; | ||
|
|
||
| const handleQuerySettingsChange = (newQuerySettings: QuerySettingsOptions[]): void => { | ||
| onChange( | ||
| produce(value, (draft: TimeSeriesChartOptions) => { | ||
| draft.querySettings = newQuerySettings; | ||
| }) | ||
| ); | ||
| }; | ||
| // Every time a new query settings input is added, we want to focus the recently added input | ||
| const recentlyAddedInputRef = useRef<HTMLInputElement | null>(null); | ||
| const focusRef = useRef(false); | ||
|
|
||
| const { queryDefinitions } = useDataQueriesContext(); | ||
| const queryNames: string[] = useMemo(() => generateQueryNames(queryDefinitions), [queryDefinitions]); | ||
|
|
||
| // Every time a new query settings input is added, focus it | ||
| useEffect(() => { | ||
| if (!recentlyAddedInputRef.current || !focusRef.current) return; | ||
| recentlyAddedInputRef.current?.focus(); | ||
| focusRef.current = false; | ||
| }, [querySettingsList?.length]); | ||
|
|
||
| const handleQueryIndexChange = (e: React.ChangeEvent<HTMLInputElement>, i: number): void => { | ||
| // Any entry still using the deprecated `queryIndex` is normalized to `queryName`, remove when queryIndex is removed | ||
| const handleQuerySettingsChange = (newQuerySettings: QuerySettingsOptions[]): void => { | ||
| onChange( | ||
| produce(value, (draft: TimeSeriesChartOptions) => { | ||
| draft.querySettings = newQuerySettings.map((qs) => normalizeToQueryName(qs, queryNames)); | ||
| }) | ||
| ); | ||
| }; | ||
|
|
||
| const handleQueryNameChange = (e: React.ChangeEvent<HTMLInputElement>, i: number): void => { | ||
| if (querySettingsList !== undefined) { | ||
| handleQuerySettingsChange( | ||
| produce(querySettingsList, (draft) => { | ||
| const querySettings = draft?.[i]; | ||
| if (querySettings) { | ||
| querySettings.queryIndex = parseInt(e.target.value); | ||
| querySettings.queryName = e.target.value; | ||
| } | ||
| }) | ||
| ); | ||
|
|
@@ -223,23 +256,20 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R | |
| }); | ||
| }; | ||
|
|
||
| const { queryDefinitions } = useDataQueriesContext(); | ||
| const queryCount = queryDefinitions.length; | ||
|
|
||
| // Compute the list of query indexes for which query settings are not already defined. | ||
| // This is to avoid already-booked indexes to still be selectable in the dropdown(s) | ||
| const availableQueryIndexes = useMemo(() => { | ||
| const bookedQueryIndexes = querySettingsList?.map((querySettings) => querySettings.queryIndex) ?? []; | ||
| const allQueryIndexes = Array.from({ length: queryCount }, (_, i) => i); | ||
| return allQueryIndexes.filter((_, queryIndex) => !bookedQueryIndexes.includes(queryIndex)); | ||
| }, [querySettingsList, queryCount]); | ||
| // Compute the list of query names for which query settings are not already defined. | ||
| // This is to avoid already-booked queries to still be selectable in the dropdown(s) | ||
| const availableQueryNames = useMemo(() => { | ||
| return queryNames.filter((name) => { | ||
| return !querySettingsList?.some((qs) => resolveQueryName(qs, queryNames) === name); | ||
| }); | ||
| }, [queryNames, querySettingsList]); | ||
|
|
||
| const firstAvailableQueryIndex = useMemo(() => { | ||
| return availableQueryIndexes[0] ?? NO_INDEX_AVAILABLE; | ||
| }, [availableQueryIndexes]); | ||
| const firstAvailableQueryName = useMemo(() => { | ||
| return availableQueryNames[0] ?? NO_QUERY_AVAILABLE; | ||
| }, [availableQueryNames]); | ||
|
|
||
| const defaultQuerySettings: QuerySettingsOptions = { | ||
| queryIndex: firstAvailableQueryIndex, | ||
| queryName: firstAvailableQueryName, | ||
| }; | ||
|
|
||
| const addQuerySettingsInput = (): void => { | ||
|
|
@@ -257,20 +287,20 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R | |
|
|
||
| return ( | ||
| <Stack> | ||
| {queryCount === 0 ? ( | ||
| {queryDefinitions.length === 0 ? ( | ||
| <Typography mb={2} fontStyle="italic"> | ||
| No query defined | ||
| </Typography> | ||
| ) : ( | ||
| querySettingsList?.length && | ||
| querySettingsList.map((querySettings, i) => ( | ||
| querySettingsList?.map((querySettings, i) => ( | ||
| <QuerySettingsInput | ||
| inputRef={i === querySettingsList.length - 1 ? recentlyAddedInputRef : undefined} | ||
| key={i} | ||
| queryName={resolveQueryName(querySettings, queryNames)} | ||
| querySettings={querySettings} | ||
| availableQueryIndexes={availableQueryIndexes} | ||
| onQueryIndexChange={(e) => { | ||
| handleQueryIndexChange(e, i); | ||
| availableQueryNames={availableQueryNames} | ||
| onQueryNameChange={(e) => { | ||
| handleQueryNameChange(e, i); | ||
| }} | ||
| onColorModeChange={(e) => handleColorModeChange(e, i)} | ||
| onColorValueChange={(color) => handleColorValueChange(color, i)} | ||
|
|
@@ -291,7 +321,7 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R | |
| /> | ||
| )) | ||
| )} | ||
| {queryCount > 0 && firstAvailableQueryIndex !== NO_INDEX_AVAILABLE && ( | ||
| {queryDefinitions.length > 0 && firstAvailableQueryName !== NO_QUERY_AVAILABLE && ( | ||
| <Button variant="contained" startIcon={<AddIcon />} sx={{ marginTop: 1 }} onClick={addQuerySettingsInput}> | ||
| Add Query Settings | ||
| </Button> | ||
|
|
@@ -302,8 +332,9 @@ export function QuerySettingsEditor(props: TimeSeriesChartOptionsEditorProps): R | |
|
|
||
| interface QuerySettingsInputProps { | ||
| querySettings: QuerySettingsOptions; | ||
| availableQueryIndexes: number[]; | ||
| onQueryIndexChange: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
| queryName: string; | ||
| availableQueryNames: string[]; | ||
| onQueryNameChange: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
| onColorModeChange: (e: React.ChangeEvent<HTMLInputElement>) => void; | ||
| onColorValueChange: (colorValue: string) => void; | ||
| onLineStyleChange: (lineStyle: string) => void; | ||
|
|
@@ -323,9 +354,10 @@ interface QuerySettingsInputProps { | |
| } | ||
|
|
||
| function QuerySettingsInput({ | ||
| querySettings: { queryIndex, colorMode, colorValue, lineStyle, areaOpacity, format }, | ||
| availableQueryIndexes, | ||
| onQueryIndexChange, | ||
| querySettings: { colorMode, colorValue, lineStyle, areaOpacity, format }, | ||
| queryName, | ||
| availableQueryNames, | ||
| onQueryNameChange, | ||
| onColorModeChange, | ||
| onColorValueChange, | ||
| onLineStyleChange, | ||
|
|
@@ -342,8 +374,8 @@ function QuerySettingsInput({ | |
| onRemoveFormat, | ||
| onFormatChange, | ||
| }: QuerySettingsInputProps): ReactElement { | ||
| // current query index should also be selectable | ||
| const selectableQueryIndexes = availableQueryIndexes.concat(queryIndex).sort((a, b) => a - b); | ||
| // current query name should also be selectable | ||
| const selectableQueryNames = availableQueryNames.slice().sort((a, b) => a.localeCompare(b)); | ||
|
|
||
| // State for dropdown menu | ||
| const [anchorEl, setAnchorEl] = useState<null | HTMLElement>(null); | ||
|
|
@@ -394,14 +426,15 @@ function QuerySettingsInput({ | |
| <TextField | ||
| select | ||
| inputRef={inputRef} | ||
| value={queryIndex} | ||
| value={queryName} | ||
| label="Query" | ||
| onChange={onQueryIndexChange} | ||
| onChange={onQueryNameChange} | ||
| sx={{ minWidth: '75px' }} | ||
| > | ||
| {selectableQueryIndexes.map((qi) => ( | ||
| <MenuItem key={`query-${qi}`} value={qi}> | ||
| #{qi + 1} | ||
| <MenuItem value={queryName}>{queryName}</MenuItem> | ||
| {selectableQueryNames.map((qn) => ( | ||
| <MenuItem key={`query-${qn}`} value={qn}> | ||
| {qn} | ||
| </MenuItem> | ||
| ))} | ||
| </TextField> | ||
|
|
@@ -414,7 +447,7 @@ function QuerySettingsInput({ | |
| <MenuItem value="fixed">Fixed</MenuItem> | ||
| </TextField> | ||
| <OptionsColorPicker | ||
| label={`Query n°${queryIndex + 1}`} | ||
| label={queryName} | ||
| color={colorValue || DEFAULT_COLOR_VALUE} | ||
| onColorChange={onColorValueChange} | ||
| /> | ||
|
|
@@ -449,7 +482,7 @@ function QuerySettingsInput({ | |
| {/* Area Opacity section */} | ||
| {areaOpacity !== undefined && ( | ||
| <SettingsSection label="Opacity" onRemove={onRemoveAreaOpacity}> | ||
| {/* Spacer as I don't want to add a prop to SettingsSection for left-padding just for that case.. */} | ||
| {/* Spacer as I don't want to add a prop to SettingsSection for left-padding just for that case. */} | ||
| <Box /> | ||
| <Slider | ||
| value={areaOpacity} | ||
|
|
@@ -506,7 +539,7 @@ function QuerySettingsInput({ | |
| </Stack> | ||
| {/* Delete Button for this query settings */} | ||
| <Box sx={{ display: 'flex', alignItems: 'center', justifyContent: 'center' }}> | ||
| <IconButton aria-label={`delete settings for query n°${queryIndex + 1}`} onClick={onDelete}> | ||
| <IconButton aria-label={`delete settings for query '${queryName}'`} onClick={onDelete}> | ||
| <DeleteIcon /> | ||
| </IconButton> | ||
| </Box> | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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.
Look at the following payload that I crafted and sent to the backend.
Since the schema is fully optional now, such a payload is persisted. I think this should be avoided. If we insist that we should keep all fields optional, then maybe we should intercept the
reqand drop thequerySettingscompletely. (I am just discussing the payload point of view)