Skip to content

Commit 49348e1

Browse files
ahorowitz123ahorowitz123
authored
feat(drawing-toolbar): modernize toolbar design (#734)
* feat(drawing-toolbar): modernize toolbar design * feat(drawing-toolbar): put changes behind modernization feature * feat(drawing-toolbar): tests * feat(drawing-toolbar): fix tests * feat(drawing-toolbar): add icons * feat(drawing-toolbar): fix package.json
1 parent 10103ac commit 49348e1

File tree

7 files changed

+206
-25
lines changed

7 files changed

+206
-25
lines changed

‎src/components/Popups/PopupDrawingToolbar.scss‎

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,7 @@
2727
}
2828
}
2929

30+
// Legacy styles (default)
3031
.ba-PopupDrawingToolbar {
3132
background-color: $bdl-gray;
3233
border-radius: $bdl-border-radius-size;
@@ -44,6 +45,19 @@
4445
}
4546
}
4647

48+
// Modernized styles (when feature flag is enabled)
49+
.ba-PopupDrawingToolbar--modernized {
50+
background-color: transparent;
51+
border-radius: 46px;
52+
53+
.ba-Popup-content {
54+
gap: 4px;
55+
padding: 8px;
56+
background-color: rgba(0, 0, 0, .8);
57+
border-radius: 46px;
58+
}
59+
}
60+
4761
.ba-PopupDrawingToolbar-group {
4862
position: relative;
4963
display: flex;
@@ -53,6 +67,15 @@
5367
& + & {
5468
border-left: 1px solid #000;
5569
}
70+
71+
.ba-PopupDrawingToolbar--modernized & {
72+
gap: 4px;
73+
}
74+
}
75+
76+
// Modernized border-left for adjacent groups
77+
.ba-PopupDrawingToolbar--modernized .ba-PopupDrawingToolbar-group + .ba-PopupDrawingToolbar-group {
78+
border-left: 1px solid rgb(255, 255, 255, 8%);
5679
}
5780

5881
.ba-PopupDrawingToolbar-delete,
@@ -65,6 +88,7 @@
6588
}
6689
}
6790

91+
// Legacy comment button styles
6892
.ba-PopupDrawingToolbar-comment {
6993
@include ba-PopupDrawingToolbarButton($width: max-content);
7094
@include common-typography;
@@ -74,3 +98,10 @@
7498
color: $white;
7599
font-weight: bold;
76100
}
101+
102+
// Modernized comment button styles
103+
.ba-PopupDrawingToolbar--modernized .ba-PopupDrawingToolbar-comment {
104+
padding-right: 8px;
105+
padding-left: 12px;
106+
font-size: 15px;
107+
}

‎src/components/Popups/PopupDrawingToolbar.tsx‎

Lines changed: 18 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,21 @@
11
import React from 'react';
2+
import { useSelector } from 'react-redux';
23
import classNames from 'classnames';
34
import { FormattedMessage, useIntl } from 'react-intl';
5+
// Legacy icons (box-ui-elements)
46
import IconRedo from 'box-ui-elements/es/icon/line/Redo16';
57
import IconTrash from 'box-ui-elements/es/icon/line/Trash16';
68
import IconUndo from 'box-ui-elements/es/icon/line/Undo16';
9+
// Modernized icons
10+
import ArrowBack from '../../icons/ArrowBack';
11+
import ArrowForward from '../../icons/ArrowForward';
12+
import Trash from '../../icons/Trash';
713
import messages from './messages';
814
import PopupBase from './PopupBase';
915
import { Options, PopupReference, Rect } from './Popper';
1016
import './PopupDrawingToolbar.scss';
17+
import { isFeatureEnabled } from '../../store/options/selectors';
18+
import { AppState } from '../../store/types';
1119

1220
export type PopupBaseRef = PopupBase;
1321

@@ -48,14 +56,20 @@ const options: Partial<Options> = {
4856
placement: 'top',
4957
};
5058

59+
const ICON_SIZE = 20;
60+
5161
const PopupDrawingToolbar = (props: Props, ref: React.Ref<PopupBaseRef>): JSX.Element => {
5262
const { canComment, canRedo, canUndo, className, onDelete, onRedo, onReply, onUndo, reference } = props;
63+
const isModernizationEnabled = useSelector((state: AppState) => isFeatureEnabled(state, 'modernization'));
64+
5365
const intl = useIntl();
5466

5567
return (
5668
<PopupBase
5769
ref={ref}
58-
className={classNames(className, 'ba-PopupDrawingToolbar')}
70+
className={classNames(className, 'ba-PopupDrawingToolbar', {
71+
'ba-PopupDrawingToolbar--modernized': isModernizationEnabled,
72+
})}
5973
data-resin-component="popupDrawingToolbar"
6074
options={options}
6175
reference={reference}
@@ -69,7 +83,7 @@ const PopupDrawingToolbar = (props: Props, ref: React.Ref<PopupBaseRef>): JSX.El
6983
title={intl.formatMessage(messages.drawingButtonUndo)}
7084
type="button"
7185
>
72-
<IconUndo />
86+
{isModernizationEnabled ? <ArrowBack height={ICON_SIZE} width={ICON_SIZE} /> : <IconUndo />}
7387
</button>
7488
<button
7589
className="ba-PopupDrawingToolbar-redo"
@@ -79,7 +93,7 @@ const PopupDrawingToolbar = (props: Props, ref: React.Ref<PopupBaseRef>): JSX.El
7993
title={intl.formatMessage(messages.drawingButtonRedo)}
8094
type="button"
8195
>
82-
<IconRedo />
96+
{isModernizationEnabled ? <ArrowForward height={ICON_SIZE} width={ICON_SIZE} /> : <IconRedo />}
8397
</button>
8498
<button
8599
className="ba-PopupDrawingToolbar-delete"
@@ -88,7 +102,7 @@ const PopupDrawingToolbar = (props: Props, ref: React.Ref<PopupBaseRef>): JSX.El
88102
title={intl.formatMessage(messages.drawingButtonDelete)}
89103
type="button"
90104
>
91-
<IconTrash />
105+
{isModernizationEnabled ? <Trash height={ICON_SIZE} width={ICON_SIZE} /> : <IconTrash />}
92106
</button>
93107
</div>
94108
<div className="ba-PopupDrawingToolbar-group">

‎src/components/Popups/__tests__/PopupDrawingToolbar-test.tsx‎

Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,13 +1,23 @@
11
import React from 'react';
2+
import * as ReactRedux from 'react-redux';
23
import IconRedo from 'box-ui-elements/es/icon/line/Redo16';
34
import IconTrash from 'box-ui-elements/es/icon/line/Trash16';
45
import IconUndo from 'box-ui-elements/es/icon/line/Undo16';
56
import { FormattedMessage } from 'react-intl';
67
import { shallow, ShallowWrapper } from 'enzyme';
8+
import ArrowBack from '../../../icons/ArrowBack';
9+
import ArrowForward from '../../../icons/ArrowForward';
10+
import Trash from '../../../icons/Trash';
711
import messages from '../messages';
812
import PopupBase from '../PopupBase';
913
import PopupDrawingToolbar, { Props } from '../PopupDrawingToolbar';
1014

15+
jest.mock('react-redux', () => ({
16+
__esModule: true,
17+
...jest.requireActual('react-redux'),
18+
useSelector: jest.fn(),
19+
}));
20+
1121
describe('PopupDrawingToolbar', () => {
1222
const getDataTestId = (button: string): string => `ba-PopupDrawingToolbar-${button}`;
1323
const getButton = (wrapper: ShallowWrapper, button: string): ShallowWrapper =>
@@ -36,6 +46,10 @@ describe('PopupDrawingToolbar', () => {
3646
const getWrapper = (props?: Partial<Props>): ShallowWrapper =>
3747
shallow(<PopupDrawingToolbar {...getDefaults()} {...props} />);
3848

49+
beforeEach(() => {
50+
jest.spyOn(ReactRedux, 'useSelector').mockImplementation(() => false);
51+
});
52+
3953
describe('render', () => {
4054
test('should render popup and buttons', () => {
4155
const wrapper = getWrapper({ className: 'foo' });
@@ -116,4 +130,48 @@ describe('PopupDrawingToolbar', () => {
116130
expect(mockFn).toHaveBeenCalled();
117131
});
118132
});
133+
134+
describe('modernization feature flag', () => {
135+
test('should render legacy icons when modernization is disabled', () => {
136+
jest.spyOn(ReactRedux, 'useSelector').mockImplementation(() => false);
137+
const wrapper = getWrapper();
138+
139+
expect(wrapper.exists(IconUndo)).toBe(true);
140+
expect(wrapper.exists(IconRedo)).toBe(true);
141+
expect(wrapper.exists(IconTrash)).toBe(true);
142+
expect(wrapper.exists(ArrowBack)).toBe(false);
143+
expect(wrapper.exists(ArrowForward)).toBe(false);
144+
expect(wrapper.exists(Trash)).toBe(false);
145+
});
146+
147+
test('should render modernized icons when modernization is enabled', () => {
148+
jest.spyOn(ReactRedux, 'useSelector').mockImplementation(() => true);
149+
const wrapper = getWrapper();
150+
151+
expect(wrapper.exists(ArrowBack)).toBe(true);
152+
expect(wrapper.exists(ArrowForward)).toBe(true);
153+
expect(wrapper.exists(Trash)).toBe(true);
154+
expect(wrapper.exists(IconUndo)).toBe(false);
155+
expect(wrapper.exists(IconRedo)).toBe(false);
156+
expect(wrapper.exists(IconTrash)).toBe(false);
157+
});
158+
159+
test('should not apply modernized class when modernization is disabled', () => {
160+
jest.spyOn(ReactRedux, 'useSelector').mockImplementation(() => false);
161+
const wrapper = getWrapper({ className: 'foo' });
162+
163+
expect(wrapper.find(PopupBase).props()).toMatchObject({
164+
className: 'foo ba-PopupDrawingToolbar',
165+
});
166+
});
167+
168+
test('should apply modernized class when modernization is enabled', () => {
169+
jest.spyOn(ReactRedux, 'useSelector').mockImplementation(() => true);
170+
const wrapper = getWrapper({ className: 'foo' });
171+
172+
expect(wrapper.find(PopupBase).props()).toMatchObject({
173+
className: 'foo ba-PopupDrawingToolbar ba-PopupDrawingToolbar--modernized',
174+
});
175+
});
176+
});
119177
});

‎src/drawing/__tests__/DrawingAnnotations-test.tsx‎

Lines changed: 28 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,11 @@ import { AnnotationDrawing } from '../../@types';
1515

1616
jest.mock('../DrawingList');
1717
jest.mock('../../utils/useVideoTiming', () => jest.fn());
18+
jest.mock('react-redux', () => ({
19+
__esModule: true,
20+
...jest.requireActual('react-redux'),
21+
useSelector: jest.fn(() => false),
22+
}));
1823
// Mock useVideoTiming hook
1924
const mockUseVideoTiming = useVideoTiming as jest.MockedFunction<typeof useVideoTiming>;
2025

@@ -23,7 +28,6 @@ const mockVideoTimingReturn = {
2328
getCurrentVideoLocation: jest.fn(),
2429
};
2530

26-
2731
describe('DrawingAnnotations', () => {
2832
const getDefaults = (): Props => ({
2933
activeAnnotationId: null,
@@ -43,11 +47,11 @@ describe('DrawingAnnotations', () => {
4347
setupDrawing: jest.fn(),
4448
stashedPathGroups: [],
4549
undoDrawingPathGroup: jest.fn(),
46-
targetType: TARGET_TYPE.PAGE
50+
targetType: TARGET_TYPE.PAGE,
4751
});
48-
const getWrapper = (props= {} as unknown): ReactWrapper => mount(<DrawingAnnotations {...getDefaults()} {...(props as Partial<Props>)} />);
52+
const getWrapper = (props = {} as unknown): ReactWrapper =>
53+
mount(<DrawingAnnotations {...getDefaults()} {...(props as Partial<Props>)} />);
4954

50-
5155
const {
5256
target: { path_groups: pathGroups },
5357
} = annotations[0];
@@ -117,12 +121,12 @@ describe('DrawingAnnotations', () => {
117121
const setStatus = jest.fn();
118122
const referenceEl = document.createElement('video');
119123
referenceEl.currentTime = 10;
120-
124+
121125
// Set up the mock before creating the wrapper
122126
mockVideoTimingReturn.isVideoSeeking = false;
123127
mockVideoTimingReturn.getCurrentVideoLocation.mockReturnValue(10000);
124128
mockUseVideoTiming.mockReturnValue(mockVideoTimingReturn);
125-
129+
126130
const wrapper = getWrapper({
127131
canShowPopupToolbar: true,
128132
drawnPathGroups: pathGroups,
@@ -222,16 +226,20 @@ describe('DrawingAnnotations', () => {
222226
expect(wrapper.exists(PopupDrawingToolbar)).toBe(false);
223227
});
224228

225-
226229
test('should render DrawingList with correct annotation for video frame', () => {
227230
const activeAnnotationId = videoAnnotations[0].id;
228231
const referenceEl = document.createElement('video');
229-
232+
230233
referenceEl.currentTime = 10;
231-
232-
const wrapper = getWrapper({ annotations: videoAnnotations, activeAnnotationId, targetType: TARGET_TYPE.FRAME, referenceEl });
234+
235+
const wrapper = getWrapper({
236+
annotations: videoAnnotations,
237+
activeAnnotationId,
238+
targetType: TARGET_TYPE.FRAME,
239+
referenceEl,
240+
});
233241
wrapper.update();
234-
242+
235243
const list = wrapper.find(DrawingList);
236244
expect(list.hasClass('ba-DrawingAnnotations-list')).toBe(true);
237245
expect(list.prop('activeId')).toBe(activeAnnotationId);
@@ -241,10 +249,6 @@ describe('DrawingAnnotations', () => {
241249
expect(wrapper.exists(PopupDrawingToolbar)).toBe(false);
242250
});
243251

244-
245-
246-
247-
248252
test('should render DrawingCreator if allowed', () => {
249253
const wrapper = getWrapper({ isCreating: true });
250254

@@ -318,7 +322,7 @@ describe('DrawingAnnotations', () => {
318322
createVideoAnnotationTests({
319323
componentName: 'DrawingAnnotations',
320324
getWrapper,
321-
findListComponent: (wrapper) => wrapper.find(DrawingList) as unknown as ReactWrapper,
325+
findListComponent: wrapper => (wrapper.find(DrawingList) as unknown) as ReactWrapper,
322326
videoAnnotations: videoAnnotations as AnnotationDrawing[],
323327
regularAnnotations: annotations as AnnotationDrawing[],
324328
activeAnnotationId: 'video_drawing_anno_2',
@@ -332,12 +336,17 @@ describe('DrawingAnnotations', () => {
332336
const referenceEl = document.createElement('video');
333337
referenceEl.currentTime = 10;
334338
referenceEl.dispatchEvent(new Event('seeking'));
335-
339+
336340
// Set up the mock to simulate seeking
337341
mockVideoTimingReturn.isVideoSeeking = true;
338342
mockUseVideoTiming.mockReturnValue(mockVideoTimingReturn);
339-
340-
const wrapper = getWrapper({ annotations: videoAnnotations, activeAnnotationId, targetType: TARGET_TYPE.FRAME, referenceEl });
343+
344+
const wrapper = getWrapper({
345+
annotations: videoAnnotations,
346+
activeAnnotationId,
347+
targetType: TARGET_TYPE.FRAME,
348+
referenceEl,
349+
});
341350
wrapper.update();
342351

343352
const list = wrapper.find(DrawingList);
@@ -348,7 +357,6 @@ describe('DrawingAnnotations', () => {
348357
expect(wrapper.exists(DrawingCreator)).toBe(false);
349358
expect(wrapper.exists(PopupDrawingToolbar)).toBe(false);
350359
});
351-
352360
});
353361

354362
describe('state changes', () => {
@@ -373,5 +381,4 @@ describe('DrawingAnnotations', () => {
373381
},
374382
);
375383
});
376-
377384
});

‎src/icons/ArrowBack.tsx‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import * as React from 'react';
2+
3+
function ArrowBack(props: React.SVGProps<SVGSVGElement>): JSX.Element {
4+
return (
5+
<svg
6+
data-testid="ArrowBack"
7+
fill="none"
8+
focusable="false"
9+
height="1em"
10+
role="img"
11+
viewBox="0 0 20 20"
12+
width="1em"
13+
{...props}
14+
>
15+
<path
16+
d="M6.91086 1.91058C7.2363 1.58515 7.76381 1.58515 8.08925 1.91058C8.41462 2.23602 8.41462 2.76355 8.08925 3.08896L5.34511 5.8331H12.5C14.1062 5.83312 15.3897 6.4396 16.2574 7.47291C17.1089 8.4871 17.5 9.83869 17.5 11.2498C17.5 12.6609 17.109 14.0125 16.2574 15.0266C15.3897 16.0599 14.1062 16.6664 12.5 16.6664H10C9.53987 16.6664 9.16679 16.2933 9.16671 15.8331C9.16671 15.3729 9.53979 14.9998 10 14.9998H12.5C13.6552 14.9998 14.4553 14.5803 14.9805 13.9549C15.5219 13.3102 15.8334 12.3699 15.8334 11.2498C15.8334 10.1296 15.5219 9.18935 14.9805 8.54469C14.4553 7.91928 13.6552 7.49979 12.5 7.49977H5.34511L8.08925 10.2439C8.41462 10.5694 8.41471 11.0969 8.08925 11.4223C7.76384 11.7477 7.23631 11.7477 6.91086 11.4223L2.7442 7.25563C2.41876 6.9302 2.41876 6.40268 2.7442 6.07725L6.91086 1.91058Z"
17+
fill="#fff"
18+
/>
19+
</svg>
20+
);
21+
}
22+
23+
export default ArrowBack;

‎src/icons/ArrowForward.tsx‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,23 @@
1+
import * as React from 'react';
2+
3+
function ArrowForward(props: React.SVGProps<SVGSVGElement>): JSX.Element {
4+
return (
5+
<svg
6+
data-testid="ArrowForward"
7+
fill="none"
8+
focusable="false"
9+
height="1em"
10+
role="img"
11+
viewBox="0 0 20 20"
12+
width="1em"
13+
{...props}
14+
>
15+
<path
16+
d="M11.9108 1.91058C12.2363 1.58515 12.7638 1.58515 13.0892 1.91058L17.2558 6.07725C17.5813 6.40268 17.5813 6.9302 17.2558 7.25563L13.0892 11.4223C12.7638 11.7477 12.2363 11.7477 11.9108 11.4223C11.5854 11.0969 11.5854 10.5694 11.9108 10.2439L14.6549 7.49977H7.5C6.34482 7.49977 5.54479 7.91927 5.01953 8.54469C4.4782 9.18935 4.16667 10.1296 4.16667 11.2498C4.16668 12.3699 4.47819 13.3102 5.01953 13.9549C5.54479 14.5803 6.34484 14.9998 7.5 14.9998H10C10.4603 14.9998 10.8333 15.3729 10.8333 15.8331C10.8333 16.2933 10.4603 16.6664 10 16.6664H7.5C5.89388 16.6664 4.6104 16.0599 3.74268 15.0266C2.89112 14.0125 2.50001 12.6609 2.5 11.2498C2.5 9.83869 2.89113 8.4871 3.74268 7.47291C4.6104 6.43958 5.89385 5.8331 7.5 5.8331H14.6549L11.9108 3.08896C11.5854 2.76354 11.5854 2.23602 11.9108 1.91058Z"
17+
fill="#fff"
18+
/>
19+
</svg>
20+
);
21+
}
22+
23+
export default ArrowForward;

0 commit comments

Comments
 (0)