feat(annotations): tooltip #1364

ghost · 2021-04-26T20:49:08Z

I see that AnnotationsControls Component is imported for Images & Docs.
this.preview.updateExperiences will be called whenever the experiences props passed in updates feat(content-preview): update props in content preview box-ui-elements#2549
- this will updateExperiences in the viewer depending on the file
  - this will renderUI again & update ControlsRoot
    - If the tooltip is showing we want the controls bar to show therefore I added forceShow prop to ControlsLayer Component
    - When the user closes the tooltip, we don't want the control bar to persist, so I added the callback setWasClosedByUser

ghost · 2021-04-26T22:41:38Z

src/lib/viewers/controls/annotations/__tests__/AnnotationsControls-test.tsx


        beforeEach(() => {
            jest.spyOn(React, 'useEffect').mockImplementation(cb => {
-                unmount = cb() as () => void; // Enzyme unmount helper does not currently invoke useEffect cleanup
+                if (!found) {


After adding TargetedClickThroughGuideTooltip there is now more than one useEffect spied on. The imported useEffect from BUIE that is being spied on is https://github.com/box/box-ui-elements/blob/master/src/features/targeting/utils/useOnClickBody.js. Therefore I modified this to select the first one

Can we mock AnnotationsTargetedTooltip, instead? It doesn't look like it's under test at the moment.

ConradJChan · 2021-04-26T22:10:04Z

src/lib/viewers/controls/annotations/AnnotationsButton.tsx

-            {...rest}
+        <TargetedClickThroughGuideTooltip
+            body={__('annotations_tooltip_body')}
+            className="preview-annotations-tooltip"


We've been using bp- as the prefix for preview classnames

We also use SUIT CSS naming conventions, so bp-AnnotationsTooltip would work here.

ConradJChan · 2021-04-26T23:07:26Z

src/lib/viewers/controls/controls-root/ControlsRoot.tsx

+        if (this.wasClosedByUser.tooltipFlowAnnotationsExperience) {
+            return false;
+        }
+
+        if (!this.experiences.tooltipFlowAnnotationsExperience) {
+            return false;
+        }
+
+        return this.experiences.tooltipFlowAnnotationsExperience.canShow;


Is there a way this logic could be enhanced to scale to support more experiences?

Let me know what you think of my solution that can be used by more experiences once I push my changes!

ConradJChan · 2021-04-26T23:10:52Z

src/lib/viewers/controls/controls-layer/ControlsLayer.tsx

    // Destroy timeouts on unmount
    React.useEffect(() => helpersRef.current.clean, []);

    return (
        <div
-            className={`bp-ControlsLayer ${isShown ? SHOW_CLASSNAME : ''}`}
+            className={`bp-ControlsLayer ${isShown || forceShow ? SHOW_CLASSNAME : ''}`}


Does forceShow need to be maintained separately from isShown state? Or can it be rolled into the state value?

ConradJChan · 2021-04-26T23:13:56Z

src/lib/viewers/controls/annotations/AnnotationsControls.tsx

@@ -76,7 +82,9 @@ export default function AnnotationsControls({
                isEnabled={showDrawing}
                mode={AnnotationMode.DRAWING}
                onClick={handleModeClick}
+                setWasClosedByUser={setWasClosedByUser}


Does setWasClosedByUser need to be passed down or can the tooltipFlowAnnotationsExperience be modified here to include setWasClosedByUser so that it can be passed transparently in the child component

jstoffan · 2021-04-26T23:29:56Z

src/lib/viewers/controls/controls-root/ControlsRoot.tsx

@@ -67,7 +82,32 @@ export default class ControlsRoot {
        this.controlsEl.classList.remove('bp-is-hidden');
    }

+    shouldForceShow(): boolean {
+        if (this.wasClosedByUser.tooltipFlowAnnotationsExperience) {


Can we move this logic to another component? ControlsRoot is currently pretty generic and shouldn't know about what controls/experiments it's rendering. I'm thinking something like:

<ControlsLayer onMount={this.handleMount}> <ControlsExperiments onClosed={this.helpers.reset()}> {controls} </ControlsExperiments> </ControlsLayer>

jstoffan · 2021-04-26T23:34:24Z

src/lib/viewers/controls/controls-layer/ControlsLayer.tsx

@@ -10,13 +10,14 @@ export type Helpers = {

 export type Props = {
    children: React.ReactNode;
+    forceShow?: boolean;


Can we avoid having a separate prop for showing/hiding? It could get difficult to keep things synced up.

jstoffan · 2021-04-26T23:34:43Z

src/lib/viewers/controls/annotations/AnnotationsControls.tsx

 };

 export default function AnnotationsControls({
    annotationColor = bdlBoxBlue,
    annotationMode = AnnotationMode.NONE,
+    experiences,


Can we avoid prop drilling by using context?

Yes, I think that's a good idea!

jstoffan · 2021-04-26T23:35:22Z

src/lib/viewers/controls/annotations/AnnotationsButton.tsx

-            {...rest}
+        <TargetedClickThroughGuideTooltip
+            body={__('annotations_tooltip_body')}
+            className="preview-annotations-tooltip"


We also use SUIT CSS naming conventions, so bp-AnnotationsTooltip would work here.

jstoffan · 2021-04-26T23:35:34Z

src/lib/viewers/controls/annotations/AnnotationsButton.tsx

-            onClick={(): void => onClick(mode)}
-            type="button"
-            {...rest}
+        <TargetedClickThroughGuideTooltip


Can this live in a separate component?

jstoffan · 2021-04-26T23:36:44Z

src/lib/Preview.js

+     * @return {void}
+     */
+    updateExperiences(experiences) {
+        this.options.experiences = experiences;


Do we need to keep a copy of this on the Preview instance, viewer instance, and controls instance? Keeping them all in sync seems to be adding some overhead.

Cool! It looks like we don't need it in Preview instance 👍 We do need it in viewer instance to pass it to the context. I think we need it in controls but will continue to look

jstoffan · 2021-04-26T23:37:45Z

src/lib/viewers/controls/annotations/AnnotationsButton.tsx

@@ -11,6 +13,8 @@ export type Props = Omit<React.ButtonHTMLAttributes<HTMLButtonElement>, 'onClick
    isEnabled?: boolean;
    mode: AnnotationMode;
    onClick?: (mode: AnnotationMode) => void;
+    setWasClosedByUser?: (experienceName: string | undefined) => void;
+    tooltipFlowAnnotationsExperience?: TargetingApi;
 };

 export default function AnnotationsButton({


Should every annotation button be wrapped in this tooltip? Will only one show?

My first way of doing it was every AnnotationsButton was wrapped in this tooltip, but it was only ever shown for one due to the shouldTarget prop. I've moved the tooltip out to the Annotations Control component. Now it only wraps one button. Let me know what you think. I wasn't sure if creating another component was a good idea. Thank you!

jstoffan · 2021-04-26T23:38:29Z

src/lib/viewers/controls/annotations/AnnotationsButton.tsx

@@ -20,22 +24,46 @@ export default function AnnotationsButton({
    isEnabled = true,
    mode,
    onClick = noop,
+    setWasClosedByUser = noop,
+    tooltipFlowAnnotationsExperience = {


Can we check for this value being defined and not render the tooltip if it's missing?

ghost · 2021-04-29T00:21:42Z

Still WIP, I'm trying to figure out how to hide/show control bar when tooltip is showing & closed

ghost · 2021-05-03T15:53:46Z

Thank you @jstoffan for your help! I referenced your example of using 2 different contexts. The only thing I had to add was a wasClosedByUser state in the AnnotationsTaretedTooltip component. This makes sure that we don't call setIsForced(true); after the tooltip is closed. There is similar logic in BUIE to make sure onShow is only called once https://github.com/box/box-ui-elements/blob/master/src/features/targeting/useShowOne.js#L14

jstoffan · 2021-04-27T17:28:24Z

src/i18n/en-US.properties

@@ -212,6 +212,10 @@ annotations_create_error=We’re sorry, the annotation could not be created.
 annotations_delete_error=We’re sorry, the annotation could not be deleted.
 # Text for when the authorization token is invalid
 annotations_authorization_error=Your session has expired. Please refresh the page.
+# Text for annotations tooltip body


Can we add that this is a first-time user experience tooltip to the description? Our localization service uses this string to help translate the content properly.

jstoffan · 2021-05-03T17:29:26Z

src/lib/viewers/controls/annotations/AnnotationsControls.tsx

 import IconDrawing24 from '../icons/IconDrawing24';
 import IconHighlightText16 from '../icons/IconHighlightText16';
 import IconRegion24 from '../icons/IconRegion24';
 import useFullscreen from '../hooks/useFullscreen';
 import { AnnotationMode } from '../../../types';
+


We typically group all imports together without whitespace.

jstoffan · 2021-05-03T17:31:58Z

src/lib/viewers/controls/annotations/AnnotationsControls.tsx

-            >
-                <IconDrawing24 fill={isDrawingActive ? annotationColor : '#fff'} />
-            </AnnotationsButton>
+            <AnnotationsTargetedTooltip isEnabled={showDrawing}>


Do we have a preference for which annotation type we want to draw the most attention to? It seems like the choice would be between drawing and regions, the latter of which may be a bit simpler of a user experience for a first-time user. It may be worth checking with the product team on this question.

I will confirm with product 👍

Thanks, it looks like they prefer the comment region button!

jstoffan · 2021-05-03T17:33:59Z

src/lib/viewers/controls/annotations/AnnotationsTargetedTooltip.tsx

@@ -0,0 +1,57 @@
+import React from 'react';
+import TargetedClickThroughGuideTooltip from 'box-ui-elements/es/features/targeting/TargetedClickThroughGuideTooltip';


It looks like this relies on react-tether. Can we ensure that we're using the same version BUIE expects? Can we also do a comparison of the production bundle size before and after this change?

I updated react-tether to 1.0.5. I will compare the bundle sizes now...

It looks like Preview.js went up about 100 Kib and Preview.css 10 Kib. Do you think that sounds reasonable?

Hm, that seems like a big jump given the nature of the change. Can we use the bundle analyzer to make sure we're not pulling in code we shouldn't be, like another version of React or something?

We should also check the built CSS file to make sure we're not bringing in extra global styles from the Button components in GuideTooltip. I think we've run into issues around that before.

preview.js is now at 755 KiB. I ran the analyzer and see that the Tooltip Component in BUIE is about 15.8 KiB, therefore I think the Tooltip component + React Tether make up most of the 54 additional KiB. I'm not sure what's included in the 10 KiB extra for preview.css. Do you know how to see what styles are included in the webpack analyzer plugin?

jstoffan · 2021-05-03T17:34:40Z

src/lib/viewers/controls/annotations/AnnotationsTargetedTooltip.tsx

+    );
+
+    if (!shouldTarget) {
+        return <div>{children}</div>;


Can we omit the wrapping div here?

jstoffan · 2021-05-03T17:38:52Z

src/lib/viewers/controls/annotations/__tests__/AnnotationsControls-test.tsx


        beforeEach(() => {
            jest.spyOn(React, 'useEffect').mockImplementation(cb => {
-                unmount = cb() as () => void; // Enzyme unmount helper does not currently invoke useEffect cleanup
+                if (!found) {


Can we mock AnnotationsTargetedTooltip, instead? It doesn't look like it's under test at the moment.

jstoffan · 2021-05-03T17:40:01Z

src/lib/viewers/controls/experiences/ExperiencesProvider.tsx

+import { TargetingApi } from '../../../types';
+
+export type Props = React.PropsWithChildren<{
+    experiences?: {


Can we use the shared Experiences type here?

src/lib/viewers/controls/annotations/AnnotationsTargetedTooltip.tsx

jstoffan · 2021-05-03T21:36:21Z

src/lib/viewers/controls/annotations/__tests__/AnnotationsTargetedTooltip-test.tsx

+
+            expect(wrapper.children().text()).not.toBe('Child');
+            expect(wrapper.children().prop('shouldTarget')).toBe(true);
+            expect(wrapper.children().prop('body')).toBeTruthy();


Can we use more explicit assertions here?

jstoffan · 2021-05-03T21:37:04Z

src/lib/viewers/controls/controls-layer/ControlsLayer.tsx

@@ -1,5 +1,7 @@
 import React from 'react';
 import noop from 'lodash/noop';
+import ControlsLayerContext from './ControlsLayerContext';
+


We typically group our imports without extra whitespace.

src/lib/viewers/controls/controls-layer/ControlsLayer.tsx

jstoffan · 2021-05-03T21:39:27Z

src/lib/viewers/controls/annotations/AnnotationsTargetedTooltip.tsx

@@ -0,0 +1,57 @@
+import React from 'react';
+import TargetedClickThroughGuideTooltip from 'box-ui-elements/es/features/targeting/TargetedClickThroughGuideTooltip';


Hm, that seems like a big jump given the nature of the change. Can we use the bundle analyzer to make sure we're not pulling in code we shouldn't be, like another version of React or something?

We should also check the built CSS file to make sure we're not bringing in extra global styles from the Button components in GuideTooltip. I think we've run into issues around that before.

jstoffan

This is looking great!

src/lib/viewers/controls/annotations/AnnotationsTargetedTooltip.tsx

src/lib/viewers/controls/annotations/__tests__/AnnotationsTargetedTooltip-test.tsx

src/lib/viewers/controls/annotations/AnnotationsTargetedTooltip.tsx

src/lib/viewers/controls/annotations/AnnotationsTargetedTooltip.scss

jstoffan

🚀

ghost self-requested a review as a code owner April 26, 2021 20:49

ghost commented Apr 26, 2021

View reviewed changes

ConradJChan reviewed Apr 26, 2021

View reviewed changes

jstoffan reviewed Apr 26, 2021

View reviewed changes

jstoffan reviewed May 3, 2021

View reviewed changes

Megan Smith added 6 commits May 4, 2021 08:57

feat(annotations): tooltip

2e67fc6

feat(annotations): tooltip

0e711fa

feat(annotations): tooltip

2139214

feat(annotations): tooltip

d15eaaf

feat(annotations): tooltip

956da1d

feat(annotations): tooltip

d99eb4f

jstoffan reviewed May 4, 2021

View reviewed changes

feat(annotations): tooltip

90bcf47

jstoffan approved these changes May 4, 2021

View reviewed changes

ghost added the ready-to-merge label May 4, 2021

mergify bot merged commit d030fe3 into box:master May 4, 2021

		@@ -0,0 +1,57 @@
		import React from 'react';
		import TargetedClickThroughGuideTooltip from 'box-ui-elements/es/features/targeting/TargetedClickThroughGuideTooltip';

feat(annotations): tooltip #1364

feat(annotations): tooltip #1364

Conversation

ghost commented Apr 26, 2021 • edited by ghost Loading

ghost Apr 26, 2021 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Apr 29, 2021

ghost commented May 3, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jstoffan left a comment

Choose a reason for hiding this comment

jstoffan left a comment

Choose a reason for hiding this comment

ghost commented Apr 26, 2021 •

edited by ghost

Loading

ghost Apr 26, 2021 •

edited by ghost

Loading