Separate trash button; fix tests

openstax · Sep 19, 2024 · 1c4033b · 1c4033b
1 parent c6f5135
commit 1c4033b
Show file tree

Hide file tree

Showing 7 changed files with 655 additions and 613 deletions.
diff --git a/src/app/content/highlights/components/ColorPicker.spec.tsx b/src/app/content/highlights/components/ColorPicker.spec.tsx
@@ -105,18 +105,6 @@ describe('ColorPicker', () => {
     expect(onChange).not.toHaveBeenCalled();
   });
 
-  it('handles having no onRemove when trashcan is clicked', () => {
-    const onChange = jest.fn();
-    const component = renderer.create(<TestContainer>
-      <ColorPicker color={highlightStyles[0].label} onChange={onChange} />
-    </TestContainer>);
-
-    const button = component.root.findByType('button');
-
-    button.props.onClick();
-    expect(onChange).not.toHaveBeenCalled();
-  });
-
   it('operates as a radiogroup', () => {
     const onChange = jest.fn();
     const onRemove = jest.fn();
@@ -125,7 +113,7 @@ describe('ColorPicker', () => {
         <ColorPicker color={highlightStyles[0].label} onChange={onChange} onRemove={onRemove} />
       </TestContainer>
     );
-    const rg = root.querySelector('fieldset') as HTMLFieldSetElement;
+    const rg = root.querySelector('fieldset > fieldset') as HTMLFieldSetElement;
 
     expect(rg).toBeTruthy();
     rg?.focus();

diff --git a/src/app/content/highlights/components/ColorPicker.tsx b/src/app/content/highlights/components/ColorPicker.tsx
@@ -82,6 +82,13 @@ function nextIdx(idx: number, itemCount: number, key: NavKeys) {
   return idx;
 }
 
+// tslint:disable-next-line:variable-name
+const FSWrapper = styled.fieldset`
+  border: 0;
+  display: flex;
+  flex-direction: row;
+`;
+
 // tslint:disable-next-line:variable-name
 const ColorPicker = ({className, ...props}: Props) => {
   const ref = React.useRef<HTMLDivElement>(null);
@@ -98,11 +105,9 @@ const ColorPicker = ({className, ...props}: Props) => {
       const destIdx = nextIdx(idx, inputs.length, event.key as NavKeys);
       const el = inputs[destIdx];
 
-      if (el) {
-        event.preventDefault();
-        el.focus();
-        el.click();
-      }
+      event.preventDefault();
+      el.focus();
+      el.click();
     },
     []
   );
@@ -120,36 +125,39 @@ const ColorPicker = ({className, ...props}: Props) => {
   React.useEffect(focusOnSelected, [focusOnSelected]);
 
   return (
-    <fieldset
-      className={className}
-      tabIndex={0}
-      ref={ref}
-      onKeyDown={handleKeyNavigation}
-      onFocus={focusOnSelected}
-      role='radiogroup'
-    >
-      <legend>Choose highlight color</legend>
-      {highlightStyles.map((style) => <ColorButton key={style.label}
-        name={style.label}
-        checked={props.multiple ? props.selected.includes(style.label) : props.color === style.label}
-        style={style}
-        size={props.size}
-        tabIndex={-1}
-        onChange={() => props.multiple
-          ? props.selected.includes(style.label)
-            ? props.onChange(props.selected.filter(not(match(style.label))))
-            : props.onChange([...props.selected, style.label])
-          : props.color === style.label
-            ? props.onRemove ? props.onRemove() : null
-            : props.onChange(style.label)}
-      />)}
+    <FSWrapper>
+      <legend>Select or clear highlight color</legend>
+      <fieldset
+        className={className}
+        tabIndex={0}
+        ref={ref}
+        onKeyDown={handleKeyNavigation}
+        onFocus={focusOnSelected}
+        role='radiogroup'
+      >
+        <legend>Choose highlight color</legend>
+        {highlightStyles.map((style) => <ColorButton key={style.label}
+          name={style.label}
+          checked={props.multiple ? props.selected.includes(style.label) : props.color === style.label}
+          style={style}
+          size={props.size}
+          tabIndex={-1}
+          onChange={() => props.multiple
+            ? props.selected.includes(style.label)
+              ? props.onChange(props.selected.filter(not(match(style.label))))
+              : props.onChange([...props.selected, style.label])
+            : props.color === style.label
+              ? props.onRemove ? props.onRemove() : null
+              : props.onChange(style.label)}
+        />)}
+      </fieldset>
       { (!hasOnRemove || props.size === 'small') ? null :
         <TrashButton
           size={props.size}
           onClick={props.onRemove}
         />
       }
-    </fieldset>
+    </FSWrapper>
   );
 };
 

diff --git a/src/app/content/highlights/components/EditCard.spec.tsx b/src/app/content/highlights/components/EditCard.spec.tsx
@@ -120,8 +120,9 @@ describe('EditCard', () => {
     const data = {
       ...highlightData,
       annotation: '',
+      color: 'red',
     };
-    const component = renderer.create(
+    const {root} = renderer.create(
       <TestContainer services={services} store={store}>
         <EditCard
           {...editCardProps}
@@ -131,7 +132,8 @@ describe('EditCard', () => {
       </TestContainer>
     );
 
-    const picker = component.root.findByType(ColorPicker);
+    const picker = root.findByType(ColorPicker);
+
     renderer.act(() => {
       picker.props.onRemove();
     });
@@ -154,14 +156,25 @@ describe('EditCard', () => {
     );
 
     const picker = component.root.findByType(ColorPicker);
-    renderer.act(() => {
-      picker.props.onRemove();
-    });
 
-    expect(editCardProps.onRemove).not.toHaveBeenCalled();
+    expect(picker.props.onRemove).toBeNull();
     mockSpyUser.mockClear();
   });
 
+  it('doesn\'t chain ColorPicker onRemove if there is no data', () => {
+    const mockSpyUser = jest.spyOn(selectAuth, 'user')
+      .mockReturnValue(formatUser(testAccountsUser));
+    const component = renderer.create(
+      <TestContainer services={services} store={store}>
+            <EditCard {...editCardProps} isActive={true} />
+      </TestContainer>
+    );
+
+    const picker = component.root.findByType(ColorPicker);
+
+    expect(picker.props.onRemove).toBeNull();
+    mockSpyUser.mockClear();
+  });
   it('doesn\'t chain ColorPicker onRemove if there is a pending note', () => {
     highlight.getStyle.mockReturnValue('red');
     const mockSpyUser = jest.spyOn(selectAuth, 'user')
@@ -186,11 +199,8 @@ describe('EditCard', () => {
     });
 
     const picker = component.root.findByType(ColorPicker);
-    renderer.act(() => {
-      picker.props.onRemove();
-    });
 
-    expect(editCardProps.onRemove).not.toHaveBeenCalled();
+    expect(picker.props.onRemove).toBeNull();
     mockSpyUser.mockClear();
   });
 

diff --git a/src/app/content/highlights/components/EditCard.tsx b/src/app/content/highlights/components/EditCard.tsx
@@ -253,10 +253,10 @@ function useOnRemove(props: EditCardProps, pendingAnnotation: string) {
   const trackDeleteHighlight = useAnalyticsEvent('deleteHighlight');
   const removeAndTrack = React.useCallback(
     () => {
-      if (props.data) {
-        onRemove();
-        trackDeleteHighlight(props.data.color);
-      }
+      const data = assertDefined(props.data, 'props.data must be defined');
+
+      onRemove();
+      trackDeleteHighlight(data.color);
     },
     [onRemove, props.data, trackDeleteHighlight]
   );