Skip to content

Commit

Permalink
feat(widget-builder): Add Display Type to hook (#81431)
Browse files Browse the repository at this point in the history
Support display type in the widget builder hook. This is the first property
that is more than just a string text field. The hook should:
- Enforce specific values
- If the value is not something that is valid, then there should be a
default

The above are achieved by passing along a `decoder` function that will
take the decoded URL props and churn out the corresponding object, which
is a DisplayType in this case. In the future we will need to expand this
to more complicated data structures.
  • Loading branch information
narsaynorath authored Nov 28, 2024
1 parent 6a1fd3b commit 9e1a3f5
Show file tree
Hide file tree
Showing 5 changed files with 149 additions and 17 deletions.
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import styled from '@emotion/styled';

import SelectField from 'sentry/components/forms/fields/selectField';
import Input from 'sentry/components/input';
import {space} from 'sentry/styles/space';
import {DisplayType} from 'sentry/views/dashboards/types';
import useWidgetBuilderState, {
BuilderStateAction,
} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState';
Expand Down Expand Up @@ -34,6 +36,25 @@ function DevBuilder() {
}
/>
</Section>
<Section>
<h1>Display Type:</h1>
<div style={{flex: 1}}>{state.displayType}</div>
<SelectField
name="displayType"
value={state.displayType}
options={Object.values(DisplayType).map(value => ({
label: value,
value,
}))}
onChange={newValue =>
dispatch({
type: BuilderStateAction.SET_DISPLAY_TYPE,
payload: newValue,
})
}
style={{width: '200px'}}
/>
</Section>
</Body>
);
}
Expand All @@ -46,14 +67,17 @@ const Body = styled('div')`
const Section = styled('section')`
display: flex;
flex-direction: row;
justify-content: space-between;
justify-content: space-around;
border: 1px solid ${p => p.theme.border};
align-items: center;
> * {
flex: 1;
}
`;

const SimpleInput = styled(Input)`
width: 100%;
flex: 1;
`;

export default DevBuilder;
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ describe('useQueryParamState', () => {
LocationFixture({query: {testField: 'initial state'}})
);

const {result} = renderHook(() => useQueryParamState('testField'));
const {result} = renderHook(() => useQueryParamState({fieldName: 'testField'}));

expect(result.current[0]).toBe('initial state');
});
Expand All @@ -35,7 +35,7 @@ describe('useQueryParamState', () => {
const mockedNavigate = jest.fn();
mockedUseNavigate.mockReturnValue(mockedNavigate);

const {result} = renderHook(() => useQueryParamState('testField'));
const {result} = renderHook(() => useQueryParamState({fieldName: 'testField'}));

act(() => {
result.current[1]('newValue');
Expand All @@ -62,4 +62,18 @@ describe('useQueryParamState', () => {
// The local state should be still reflect the new value
expect(result.current[0]).toBe('newValue');
});

it('should use the decoder function to decode the query param value if provided', () => {
mockedUseLocation.mockReturnValue(
LocationFixture({query: {testField: 'initial state'}})
);

const testDecoder = (value: string) => `${value.toUpperCase()} - decoded`;

const {result} = renderHook(() =>
useQueryParamState({fieldName: 'testField', decoder: testDecoder})
);

expect(result.current[0]).toBe('INITIAL STATE - decoded');
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,22 @@ import useLocationQuery from 'sentry/utils/url/useLocationQuery';
import {useLocation} from 'sentry/utils/useLocation';
import {useNavigate} from 'sentry/utils/useNavigate';

export function useQueryParamState(
fieldName: string
): [string | undefined, (newField: string | undefined) => void] {
interface UseQueryParamStateProps<T> {
fieldName: string;
decoder?: (value: string) => T;
}

/**
* Hook to manage a state that is synced with a query param in the URL
*
* @param fieldName - The name of the query param to sync with the state
* @param decoder - A function to decode the query param value into the desired type
* @returns A tuple containing the current state and a function to update the state
*/
export function useQueryParamState<T = string>({
fieldName,
decoder,
}: UseQueryParamStateProps<T>): [T | undefined, (newField: T | undefined) => void] {
const navigate = useNavigate();
const location = useLocation();

Expand All @@ -19,15 +32,20 @@ export function useQueryParamState(
[fieldName]: decodeScalar,
},
});
const [localState, setLocalState] = useState<string | undefined>(
parsedQueryParams[fieldName]
);
const [localState, setLocalState] = useState<T | undefined>(() => {
return decoder
? decoder(parsedQueryParams[fieldName])
: // TODO(nar): This is a temporary fix to avoid type errors
// When the decoder isn't provided, we should return the value
// if T is a string, or else return undefined
(parsedQueryParams[fieldName] as T);
});

// Debounce the update to the URL query params
// to avoid unnecessary re-renders
const updateQueryParam = useMemo(
() =>
debounce((newField: string | undefined) => {
debounce((newField: T | undefined) => {
navigate({
...location,
query: {...location.query, [fieldName]: newField},
Expand All @@ -37,7 +55,7 @@ export function useQueryParamState(
);

const updateField = useCallback(
(newField: string | undefined) => {
(newField: T | undefined) => {
setLocalState(newField);
updateQueryParam(newField);
},
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import {act, renderHook} from 'sentry-test/reactTestingLibrary';

import {useLocation} from 'sentry/utils/useLocation';
import {useNavigate} from 'sentry/utils/useNavigate';
import {DisplayType} from 'sentry/views/dashboards/types';
import useWidgetBuilderState, {
BuilderStateAction,
} from 'sentry/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState';
Expand Down Expand Up @@ -70,4 +71,52 @@ describe('useWidgetBuilderState', () => {
})
);
});

describe('display type', () => {
it('returns the display type from the query params', () => {
mockedUsedLocation.mockReturnValue(
LocationFixture({
query: {displayType: DisplayType.AREA},
})
);

const {result} = renderHook(() => useWidgetBuilderState());

expect(result.current.state.displayType).toBe(DisplayType.AREA);
});

it('returns a default display type from the query params when the display type is not valid', () => {
mockedUsedLocation.mockReturnValue(
LocationFixture({
query: {displayType: 'invalid'},
})
);

const {result} = renderHook(() => useWidgetBuilderState());

expect(result.current.state.displayType).toBe(DisplayType.TABLE);
});

it('sets the display type in the query params', () => {
const mockNavigate = jest.fn();
mockedUseNavigate.mockReturnValue(mockNavigate);

const {result} = renderHook(() => useWidgetBuilderState());

act(() => {
result.current.dispatch({
type: BuilderStateAction.SET_DISPLAY_TYPE,
payload: DisplayType.AREA,
});
});

jest.runAllTimers();

expect(mockNavigate).toHaveBeenCalledWith(
expect.objectContaining({
query: expect.objectContaining({displayType: DisplayType.AREA}),
})
);
});
});
});
Original file line number Diff line number Diff line change
@@ -1,29 +1,42 @@
import {useCallback, useMemo} from 'react';

import {DisplayType} from 'sentry/views/dashboards/types';
import {useQueryParamState} from 'sentry/views/dashboards/widgetBuilder/hooks/useQueryParamState';

export const BuilderStateAction = {
SET_TITLE: 'SET_TITLE',
SET_DESCRIPTION: 'SET_DESCRIPTION',
SET_DISPLAY_TYPE: 'SET_DISPLAY_TYPE',
} as const;

type WidgetAction =
| {payload: string; type: typeof BuilderStateAction.SET_TITLE}
| {payload: string; type: typeof BuilderStateAction.SET_DESCRIPTION};
| {payload: string; type: typeof BuilderStateAction.SET_DESCRIPTION}
| {payload: DisplayType; type: typeof BuilderStateAction.SET_DISPLAY_TYPE};

interface WidgetBuilderState {
description?: string;
displayType?: DisplayType;
title?: string;
}

function useWidgetBuilderState(): {
dispatch: (action: WidgetAction) => void;
state: WidgetBuilderState;
} {
const [title, setTitle] = useQueryParamState('title');
const [description, setDescription] = useQueryParamState('description');
const [title, setTitle] = useQueryParamState<string>({fieldName: 'title'});
const [description, setDescription] = useQueryParamState<string>({
fieldName: 'description',
});
const [displayType, setDisplayType] = useQueryParamState<DisplayType>({
fieldName: 'displayType',
decoder: decodeDisplayType,
});

const state = useMemo(() => ({title, description}), [title, description]);
const state = useMemo(
() => ({title, description, displayType}),
[title, description, displayType]
);

const dispatch = useCallback(
(action: WidgetAction) => {
Expand All @@ -34,11 +47,14 @@ function useWidgetBuilderState(): {
case BuilderStateAction.SET_DESCRIPTION:
setDescription(action.payload);
break;
case BuilderStateAction.SET_DISPLAY_TYPE:
setDisplayType(action.payload);
break;
default:
break;
}
},
[setTitle, setDescription]
[setTitle, setDescription, setDisplayType]
);

return {
Expand All @@ -47,4 +63,15 @@ function useWidgetBuilderState(): {
};
}

/**
* Decodes the display type from the query params
* Returns the default display type if the value is not a valid display type
*/
function decodeDisplayType(value: string): DisplayType {
if (Object.values(DisplayType).includes(value as DisplayType)) {
return value as DisplayType;
}
return DisplayType.TABLE;
}

export default useWidgetBuilderState;

0 comments on commit 9e1a3f5

Please sign in to comment.