From 9e1a3f5add0ff6e6d04a3db897122831e14e19d9 Mon Sep 17 00:00:00 2001 From: Nar Saynorath Date: Thu, 28 Nov 2024 14:40:17 -0500 Subject: [PATCH] feat(widget-builder): Add Display Type to hook (#81431) 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. --- .../widgetBuilder/components/devBuilder.tsx | 28 ++++++++++- .../hooks/useQueryParamState.spec.tsx | 18 ++++++- .../hooks/useQueryParamState.tsx | 34 ++++++++++--- .../hooks/useWidgetBuilderState.spec.tsx | 49 +++++++++++++++++++ .../hooks/useWidgetBuilderState.tsx | 37 ++++++++++++-- 5 files changed, 149 insertions(+), 17 deletions(-) diff --git a/static/app/views/dashboards/widgetBuilder/components/devBuilder.tsx b/static/app/views/dashboards/widgetBuilder/components/devBuilder.tsx index 2fce07fc2981ce..adab56abb37a76 100644 --- a/static/app/views/dashboards/widgetBuilder/components/devBuilder.tsx +++ b/static/app/views/dashboards/widgetBuilder/components/devBuilder.tsx @@ -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'; @@ -34,6 +36,25 @@ function DevBuilder() { } /> +
+

Display Type:

+
{state.displayType}
+ ({ + label: value, + value, + }))} + onChange={newValue => + dispatch({ + type: BuilderStateAction.SET_DISPLAY_TYPE, + payload: newValue, + }) + } + style={{width: '200px'}} + /> +
); } @@ -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; diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useQueryParamState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useQueryParamState.spec.tsx index c1c2a2afb522e2..f230a22654a19b 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useQueryParamState.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useQueryParamState.spec.tsx @@ -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'); }); @@ -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'); @@ -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'); + }); }); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useQueryParamState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useQueryParamState.tsx index d284309debf1ad..220ec0418eb2be 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useQueryParamState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useQueryParamState.tsx @@ -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 { + 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({ + fieldName, + decoder, +}: UseQueryParamStateProps): [T | undefined, (newField: T | undefined) => void] { const navigate = useNavigate(); const location = useLocation(); @@ -19,15 +32,20 @@ export function useQueryParamState( [fieldName]: decodeScalar, }, }); - const [localState, setLocalState] = useState( - parsedQueryParams[fieldName] - ); + const [localState, setLocalState] = useState(() => { + 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}, @@ -37,7 +55,7 @@ export function useQueryParamState( ); const updateField = useCallback( - (newField: string | undefined) => { + (newField: T | undefined) => { setLocalState(newField); updateQueryParam(newField); }, diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx index 069548906acc3d..df80108fb0bc2c 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.spec.tsx @@ -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'; @@ -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}), + }) + ); + }); + }); }); diff --git a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx index 93ce1ad63cbc37..9fb0bbfc7b4a9b 100644 --- a/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx +++ b/static/app/views/dashboards/widgetBuilder/hooks/useWidgetBuilderState.tsx @@ -1,18 +1,22 @@ 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; } @@ -20,10 +24,19 @@ function useWidgetBuilderState(): { dispatch: (action: WidgetAction) => void; state: WidgetBuilderState; } { - const [title, setTitle] = useQueryParamState('title'); - const [description, setDescription] = useQueryParamState('description'); + const [title, setTitle] = useQueryParamState({fieldName: 'title'}); + const [description, setDescription] = useQueryParamState({ + fieldName: 'description', + }); + const [displayType, setDisplayType] = useQueryParamState({ + 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) => { @@ -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 { @@ -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;