Browse Source

ref(compositeSelect): Pass option object to `onChange` callback (#44253)

Rather than passing selected option values (type `React.Key`) to the
`onChange` callback, `CompositeSelect` should pass option objects (type
`SelectOption`). That's how `CompactSelect` and `SelectControl` do it.

**Before ——**
<img width="477" alt="Screenshot 2023-02-07 at 11 30 15 AM"
src="https://user-images.githubusercontent.com/44172267/217346219-5815cc93-d15a-46a7-b21f-365bb1fd7aeb.png">

**After ——** a bit more verbose but it's consistent
<img width="477" alt="Screenshot 2023-02-07 at 11 30 35 AM"
src="https://user-images.githubusercontent.com/44172267/217346295-8e2c60f1-9a28-46b5-a000-45e5c3b1dc33.png">
Vu Luong 2 years ago
parent
commit
7b769e45ce

+ 4 - 2
static/app/components/compactSelect/composite.spec.tsx

@@ -165,7 +165,7 @@ describe('CompactSelect', function () {
     userEvent.click(screen.getByRole('option', {name: 'Choice One'}));
 
     // Region 1's callback is called, and trigger label is updated
-    expect(region1Mock).toHaveBeenCalledWith('choice_one');
+    expect(region1Mock).toHaveBeenCalledWith({value: 'choice_one', label: 'Choice One'});
     expect(screen.getByRole('button', {name: 'Choice One'})).toBeInTheDocument();
 
     // open the menu again
@@ -199,7 +199,9 @@ describe('CompactSelect', function () {
       'aria-selected',
       'true'
     );
-    expect(region2Mock).toHaveBeenCalledWith(['choice_three']);
+    expect(region2Mock).toHaveBeenCalledWith([
+      {value: 'choice_three', label: 'Choice Three'},
+    ]);
     expect(screen.getByRole('button', {name: 'Choice One +1'})).toBeInTheDocument();
   });
 

+ 4 - 14
static/app/components/compactSelect/composite.tsx

@@ -23,12 +23,7 @@ interface BaseCompositeSelectRegion<Value extends React.Key> {
  */
 export interface SingleCompositeSelectRegion<Value extends React.Key>
   extends BaseCompositeSelectRegion<Value>,
-    Omit<
-      SingleListBoxProps<Value>,
-      'children' | 'items' | 'compositeIndex' | 'size' | 'onChange'
-    > {
-  onChange: (value: Value) => void;
-}
+    Omit<SingleListBoxProps<Value>, 'children' | 'items' | 'compositeIndex' | 'size'> {}
 
 /**
  * A multiple-selection (multiple options can be selected at the same time) "region"
@@ -38,12 +33,7 @@ export interface SingleCompositeSelectRegion<Value extends React.Key>
  */
 export interface MultipleCompositeSelectRegion<Value extends React.Key>
   extends BaseCompositeSelectRegion<Value>,
-    Omit<
-      MultipleListBoxProps<Value>,
-      'children' | 'items' | 'compositeIndex' | 'size' | 'onChange'
-    > {
-  onChange: (values: Value[]) => void;
-}
+    Omit<MultipleListBoxProps<Value>, 'children' | 'items' | 'compositeIndex' | 'size'> {}
 
 /**
  * A "region" inside a composite select. Each "region" is a separated, self-contained
@@ -158,7 +148,7 @@ function Region<Value extends React.Key>({
         value,
         defaultValue,
         closeOnSelect,
-        onChange: opts => onChange?.(opts.map(opt => opt.value)),
+        onChange,
       };
     }
     return {
@@ -166,7 +156,7 @@ function Region<Value extends React.Key>({
       value,
       defaultValue,
       closeOnSelect,
-      onChange: opt => onChange?.(opt ? opt.value : null),
+      onChange,
     };
   }, [multiple, value, defaultValue, onChange, closeOnSelect]);
 

+ 4 - 4
static/app/components/profiling/flamegraph/flamegraphToolbar/flamegraphOptionsMenu.tsx

@@ -41,10 +41,10 @@ function FlamegraphOptionsMenu({
             ...opt,
             disabled: opt.value === 'transaction' && type === 'flamegraph',
           }))}
-          onChange={value =>
+          onChange={opt =>
             dispatch({
               type: 'set xAxis',
-              payload: value,
+              payload: opt.value,
             })
           }
         />
@@ -52,10 +52,10 @@ function FlamegraphOptionsMenu({
           label={t('Color Coding')}
           value={colorCoding}
           options={colorCodingOptions}
-          onChange={value =>
+          onChange={opt =>
             dispatch({
               type: 'set color coding',
-              payload: value,
+              payload: opt.value,
             })
           }
         />

+ 3 - 3
static/app/components/replays/replayController.tsx

@@ -127,7 +127,7 @@ function ReplayOptionsMenu({speedOptions}: {speedOptions: number[]}) {
       <CompositeSelect.Region
         label={t('Playback Speed')}
         value={speed}
-        onChange={val => setSpeed(val)}
+        onChange={opt => setSpeed(opt.value)}
         options={speedOptions.map(option => ({
           label: `${option}x`,
           value: option,
@@ -137,8 +137,8 @@ function ReplayOptionsMenu({speedOptions}: {speedOptions: number[]}) {
         aria-label={t('Fast-Forward Inactivity')}
         multiple
         value={isSkippingInactive ? [SKIP_OPTION_VALUE] : []}
-        onChange={value => {
-          toggleSkipInactive(value.length > 0);
+        onChange={opts => {
+          toggleSkipInactive(opts.length > 0);
         }}
         options={[
           {

+ 2 - 2
static/app/views/performance/landing/widgets/components/widgetContainer.tsx

@@ -314,14 +314,14 @@ export const WidgetContainerActions = ({
         label={t('Display')}
         options={menuOptions}
         value={chartSetting}
-        onChange={setChartSetting}
+        onChange={opt => setChartSetting(opt.value)}
       />
       {chartDefinition.allowsOpenInDiscover && (
         <CompositeSelect.Region
           label={t('Other')}
           options={[{label: t('Open in Discover'), value: 'open_in_discover'}]}
           value=""
-          onChange={handleWidgetActionChange}
+          onChange={opt => handleWidgetActionChange(opt.value)}
         />
       )}
     </CompositeSelect>