chore: pr comments

box · May 5, 2021 · cd35cfd · cd35cfd
1 parent 6b0c918
commit cd35cfd
Show file tree

Hide file tree

Showing 9 changed files with 19 additions and 59 deletions.
diff --git a/src/lib/viewers/box3d/model3d/Model3DViewer.js b/src/lib/viewers/box3d/model3d/Model3DViewer.js
@@ -146,11 +146,7 @@ class Model3DViewer extends Box3DViewer {
      */
     handleSelectAnimationClip(clipId) {
         this.renderer.setAnimationClip(clipId);
-
-        if (this.controls && this.getViewerOption('useReactControls')) {
-            this.setAnimationState(false);
-            this.renderUI();
-        }
+        this.setAnimationState(false);
     }
 
     /**
@@ -300,15 +296,15 @@ class Model3DViewer extends Box3DViewer {
      */
     handleToggleAnimation(play) {
         this.setAnimationState(play);
-
-        if (this.controls && this.getViewerOption('useReactControls')) {
-            this.renderUI();
-        }
     }
 
     setAnimationState(play) {
         this.isAnimationPlaying = play;
         this.renderer.toggleAnimation(this.isAnimationPlaying);
+
+        if (this.controls && this.getViewerOption('useReactControls')) {
+            this.renderUI();
+        }
     }
 
     /**

diff --git a/src/lib/viewers/controls/model3d/AnimationClipsControl.scss b/src/lib/viewers/controls/model3d/AnimationClipsControl.scss
@@ -6,17 +6,14 @@
     .bp-SettingsFlyout {
         right: auto;
         left: 0;
-        max-height: 200px;
-        overflow-x: hidden;
-        overflow-y: auto;
+        max-width: 240px;
     }
 }
 
 .bp-AnimationClipsControl-radioItem {
     .bp-SettingsRadioItem-value {
-        max-width: 183px;
+        max-width: 180px;
         overflow-x: hidden;
-        font-size: 13px;
         white-space: nowrap;
         text-overflow: ellipsis;
     }

diff --git a/src/lib/viewers/controls/model3d/AnimationClipsControl.tsx b/src/lib/viewers/controls/model3d/AnimationClipsControl.tsx
@@ -19,7 +19,7 @@ export const padLeft = (x: number, width: number): string => {
     return x.length >= width ? x : new Array(width - x.length + 1).join('0') + x;
 };
 
-export const formatDurationStr = (duration: number): string => {
+export const formatDuration = (duration: number): string => {
     let secondsLeft = Math.floor(duration);
     const hours = Math.floor(secondsLeft / 3600);
     const hoursStr = padLeft(hours.toString(), 2);
@@ -40,16 +40,15 @@ export default function AnimationClipsControl({
     onAnimationClipSelect,
 }: Props): JSX.Element {
     return (
-        <Settings className="bp-AnimationClipsControl" disableTransitions={false} icon={AnimationClipsToggle}>
+        <Settings className="bp-AnimationClipsControl" icon={AnimationClipsToggle}>
             <Settings.Menu name={Menu.MAIN}>
                 {animationClips.map(({ duration, id, name }) => {
-                    const isSelected = id === currentAnimationClipId;
                     return (
                         <Settings.RadioItem
                             key={id}
                             className="bp-AnimationClipsControl-radioItem"
-                            isSelected={isSelected}
-                            label={`${formatDurationStr(duration)} ${name}`}
+                            isSelected={id === currentAnimationClipId}
+                            label={`${formatDuration(duration)} ${name}`}
                             onChange={onAnimationClipSelect}
                             value={id}
                         />

diff --git a/src/lib/viewers/controls/model3d/__tests__/AnimationClipsControl-test.tsx b/src/lib/viewers/controls/model3d/__tests__/AnimationClipsControl-test.tsx
@@ -22,7 +22,6 @@ describe('AnimationClipsControl', () => {
 
             expect(wrapper.find(Settings).props()).toMatchObject({
                 className: 'bp-AnimationClipsControl',
-                disableTransitions: false,
                 icon: AnimationClipsToggle,
             });
             expect(wrapper.exists(Settings.Menu)).toBe(true);

diff --git a/src/lib/viewers/controls/settings/Settings.tsx b/src/lib/viewers/controls/settings/Settings.tsx
@@ -11,14 +11,12 @@ import { decodeKeydown } from '../../../util';
 
 export type Props = React.PropsWithChildren<{
     className?: string;
-    disableTransitions?: boolean;
     icon?: React.ReactNode;
 }>;
 
 export default function Settings({
     children,
     className,
-    disableTransitions = false,
     icon: ToggleIcon = SettingsToggle,
     ...rest
 }: Props): JSX.Element | null {
@@ -88,9 +86,7 @@ export default function Settings({
         >
             <SettingsContext.Provider value={{ activeMenu, activeRect, setActiveMenu, setActiveRect }}>
                 <ToggleIcon ref={buttonElRef} isOpen={isOpen} onClick={handleClick} />
-                <SettingsFlyout disableTransitions={disableTransitions} isOpen={isOpen}>
-                    {children}
-                </SettingsFlyout>
+                <SettingsFlyout isOpen={isOpen}>{children}</SettingsFlyout>
             </SettingsContext.Provider>
         </div>
     );

diff --git a/src/lib/viewers/controls/settings/SettingsFlyout.tsx b/src/lib/viewers/controls/settings/SettingsFlyout.tsx
@@ -5,20 +5,14 @@ import './SettingsFlyout.scss';
 
 export type Props = React.PropsWithChildren<{
     className?: string;
-    disableTransitions?: boolean;
     isOpen: boolean;
 }>;
 
-export default function SettingsFlyout({
-    children,
-    className,
-    disableTransitions = false,
-    isOpen,
-}: Props): JSX.Element {
+export default function SettingsFlyout({ children, className, isOpen }: Props): JSX.Element {
     const [isTransitioning, setIsTransitioning] = React.useState(false);
     const flyoutElRef = React.useRef<HTMLDivElement>(null);
-    const { activeRect = { height: 'auto', width: 'auto' } } = React.useContext(SettingsContext);
-    const { height, width } = disableTransitions ? { height: 'auto', width: 'auto' } : activeRect;
+    const { activeRect } = React.useContext(SettingsContext);
+    const { height, width } = activeRect || { height: 'auto', width: 'auto' };
 
     React.useEffect(() => {
         const { current: flyoutEl } = flyoutElRef;

diff --git a/src/lib/viewers/controls/settings/SettingsRadioItem.tsx b/src/lib/viewers/controls/settings/SettingsRadioItem.tsx
@@ -16,6 +16,7 @@ export type Value = boolean | number | string;
 
 function SettingsRadioItem<V extends Value>(props: Props<V>, ref: React.Ref<Ref>): JSX.Element {
     const { className, isSelected, label, onChange, value } = props;
+    const displayedValue = label || value;
 
     const handleClick = (): void => {
         onChange(value);
@@ -46,7 +47,9 @@ function SettingsRadioItem<V extends Value>(props: Props<V>, ref: React.Ref<Ref>
             <div className="bp-SettingsRadioItem-check">
                 <IconCheckMark24 className="bp-SettingsRadioItem-check-icon" height={16} width={16} />
             </div>
-            <div className="bp-SettingsRadioItem-value">{label || value}</div>
+            <div className="bp-SettingsRadioItem-value" title={displayedValue}>
+                {displayedValue}
+            </div>
         </div>
     );
 }

diff --git a/src/lib/viewers/controls/settings/__tests__/Settings-test.tsx b/src/lib/viewers/controls/settings/__tests__/Settings-test.tsx
@@ -94,20 +94,6 @@ describe('Settings', () => {
             expect(wrapper.exists(SettingsToggle)).toBe(true);
         });
 
-        describe('disableTransitions prop', () => {
-            test('should default disableTransitions to false', () => {
-                const wrapper = getWrapper();
-
-                expect(wrapper.find(SettingsFlyout).prop('disableTransitions')).toBe(false);
-            });
-
-            test('should pass disableTransitions to along to SettingsFlyout', () => {
-                const wrapper = getWrapper({ disableTransitions: true });
-
-                expect(wrapper.find(SettingsFlyout).prop('disableTransitions')).toBe(true);
-            });
-        });
-
         describe('icon prop', () => {
             // eslint-disable-next-line @typescript-eslint/no-unused-vars
             function CustomIcon({ isOpen, ...rest }: object, ref: React.Ref<HTMLDivElement>): JSX.Element {

diff --git a/src/lib/viewers/controls/settings/__tests__/SettingsFlyout-test.tsx b/src/lib/viewers/controls/settings/__tests__/SettingsFlyout-test.tsx
@@ -60,15 +60,5 @@ describe('SettingsFlyout', () => {
 
             expect(wrapper.getDOMNode()).toHaveClass('bp-SettingsFlyout');
         });
-
-        test('should only return default height and width if disableTransitions is true', () => {
-            const activeRect = { bottom: 0, left: 0, height: 100, right: 0, top: 0, width: 100 };
-            const wrapper = getWrapper({ disableTransitions: true }, { activeRect });
-
-            expect(wrapper.childAt(0).prop('style')).toEqual({
-                height: 'auto',
-                width: 'auto',
-            });
-        });
     });
 });