feat: add "active" state to highlight on PdfViewer by fukudasjp · Pull Request #259 · watson-developer-cloud/discovery-components

fukudasjp · 2022-01-11T13:13:36Z

What do these changes do/fix?

Add "active" state to highlights on PdfViewer. When you set a highlight as active,

the highlight is displayed in darker color, and
PdfViewer reveals the highlight by changing current page and scrolling

How do you test/verify these changes?

We can use Storybook (DocumentPreview > components > PdfViewerWithHighlight > default). In the story's Knobs tab, you can set Highlights and Active highlight index to the component.

Have you documented your changes (if necessary)?

Are there any breaking changes included in this pull request?

no

jhpedemonte · 2022-01-19T22:10:30Z

Functionality is good, but I'm concerned about the "active" color not meeting accessibility standards. It's hard to gauge since it's an overlay with opacity, but I used the Digital Color Meter app from the MacOS Developer Tools to measure both the overlay color and the text color (since the overlay changes the color of the underlying text):

I then ran those numbers through the Contrast Checker:

If this were true system-level text highlighting, then the text would be true black, which easily passes all of the contrast checks. But since the overlay changes the text color, one test fails. We'll need to find a different color to use.

Or we can find a different way to denote "active"; for example, with a "focus" rectangle around it.

jhpedemonte · 2022-01-19T22:12:40Z

If using the "Active highlight index" knob, should setting it to 2 force the PDF viewer to load the 3rd page, which contains this highlight index?

Update: Oh, after refreshing Storybook, it worked. But then going back to 1 and 0 did nothing.

jhpedemonte

I keep getting confused between PdfViewerHighlight.tsx and PdfViewerWithHighlight.tsx. I think one of those should be renamed, at least, to make things clearer.

jhpedemonte · 2022-01-19T22:19:30Z

+        const active = shape?.highlightId ? activeIds?.includes(shape.highlightId) : false;
+        return (
+          <Highlight
+            key={shape?.highlightId || `highlight_${hlIndex}`}


How often will the "else" case resolve here? We shouldn't use array index for the React key -- that can easily lead to bugs.

The shape.highlightId is currently optional, but I refactor the code to make the highlightId required.

jhpedemonte · 2022-01-19T22:20:41Z

+  function getPositionStyle(bbox: Bbox, scale: number, padding: number = 0) {
+    const [left, top, right, bottom] = bbox;
+    return {
+      left: `${(left - padding) * scale}px`,
+      top: `${(top - padding) * scale}px`,
+      width: `${(right - left + padding) * scale}px`,
+      height: `${(bottom - top + padding) * scale}px`
+    };
+  }


Move this function outside of the component, since it's standalone. No sense in redefining it each time the component renders.

jhpedemonte · 2022-01-19T22:21:07Z

  return (
-    <div className={cx(`${settings.prefix}--document-preview-pdf-viewer-highlight`, className)}>
-      {highlightBoxes.map((hl, hlIndex) => {
+    <div key={highlightId} data-highlight-id={highlightId}>


Same issue here with key.

jhpedemonte · 2022-01-19T22:21:22Z

-            })}
-          </React.Fragment>
+          <div
+            key={`hlId_${index}`}


jhpedemonte · 2022-01-19T22:23:09Z


+function useScrollIntoActiveHighlight(
+  highlightDivRef: React.MutableRefObject<HTMLDivElement | null>,
+  shapes: (HighlightShape | undefined)[],


shapes can be an array of undefined?

no. I fixed the code

jhpedemonte · 2022-01-19T22:25:24Z

+          `[data-highlight-id=${activeShape.highlightId}]`
+        );
+        highlightElm?.firstElementChild?.scrollIntoView({ block: 'nearest', inline: 'nearest' });
+      }, 50);


Why 50 for the timeout? I don't like having arbitrary timeout values, since they may work on your computer, but may fail on a slower computer.

Using zero is fine since that's useful to make some code asynchronous. But it would be better to key off of something else rather than a random timeout, if possible.

The intention was to push a new task to the javascript runtime. So I use the value 0.

jhpedemonte · 2022-01-19T22:27:04Z

+        );
+        highlightElm?.firstElementChild?.scrollIntoView({ block: 'nearest', inline: 'nearest' });
+      }, 50);
+      return () => clearTimeout(timer);


If I'm reading this right, this is meant to be the cleanup function for the useEffect, right? If so, please comment.

Or better yet, do it like they show in the docs:

return function cleanup() { clearTimeout(timer); }

jhpedemonte · 2022-01-19T22:45:34Z

+  const [highlightPage, setHighlightPage] = useState<number | undefined>();
+
+  useEffect(() => {
+    const pages = activeHighlightPages.filter(nonEmpty);


Wouldn't it make more sense to move this into useActiveHighlightPages?

jhpedemonte · 2022-01-19T22:48:04Z

+    setHighlightPage(newPage);
+  }, [page, activeHighlightPages]);
+
+  const [currentPage, setCurrentPage] = useState(page);


I think this function would read better if this were moved to the top and it referenced currentPage instead of page in the rest of the code.

Moved the currentPage useState to the top, revised the code and added comments.

jhpedemonte · 2022-01-19T22:50:19Z

    fieldText = documentFieldArray?.[index ?? 0];
  }
-  if (!fieldText || !span) {
+  if (typeof fieldText?.['table_text'] === 'string') {


Why this change?

Highlighting went wrong when there is table field in a document because it contains an object instead of text. Also, there is text_mappings for between bboxes and text in table fields. That code is to specially handles the table field to get field text referred in the text_mappings.

As the code is not clear, I updated the code and added a comment.

fukudasjp · 2022-01-20T04:51:54Z

@jhpedemonte thanks for heading up the contrast issue! I updated the highlight style (8cb9c3f) to use smaller opacity value for the style not to affect the original text color. That passes the test.

…hlight

fukudasjp · 2022-01-20T05:44:41Z

Oh, after refreshing Storybook, it worked. But then going back to 1 and 0 did nothing.

This issue came from the Storybook code and fixed it.

lgtm-com · 2022-01-20T08:31:06Z

This pull request introduces 1 alert when merging 01d4ff7 into 5026722 - view on LGTM.com

new alerts:

1 for Unneeded defensive code

fukudasjp · 2022-01-20T09:39:38Z

I keep getting confused between PdfViewerHighlight.tsx and PdfViewerWithHighlight.tsx. I think one of those should be renamed, at least, to make things clearer.

I agree. Just renaming PdfViewerHighlight to PdfHighlight makes the situation much better and it's the first step, I think. I drafted #275 on top of this PR.

* origin/master: fix: add missing prop to doc provider interface (#283) chore: publish v1.5.0-beta.15 [ci skip] fix: fix PDFViewer rendering issue (#267) chore: publish v1.5.0-beta.14 [ci skip] feat: update preview toolbar design for PDF viewer (#251) chore: publish v1.5.0-beta.13 [ci skip] fix: Fix PdfView alignment in DocumentPreview (#256) chore: publish v1.5.0-beta.12 [ci skip] refactor: rename PdfViewerHighlight to PdfHighlight (#275) chore: publish v1.5.0-beta.11 [ci skip] feat: add "active" state to highlight on PdfViewer (#259) chore: update lockfile chore: update CODEOWNERS

fukudasjp added 5 commits January 11, 2022 10:28

feat: add active highlight to PDFViewer

aa60f56

feat: add storybook

098b40b

chore: update yarn.lock

2b7f7cd

fix: fix paging issue

31617dd

fix: fix errors in console

fa6181f

fukudasjp requested review from JohnWColby, broulaye, jhpedemonte and noah-eigenfeld as code owners January 11, 2022 13:13

fukudasjp changed the title ~~feat: add active (selected) highlight feature to PdfViewer~~ feat: add active highlight to PdfViewer Jan 11, 2022

fix: fix highligt scrollInto feature

7d50d1f

fukudasjp changed the title ~~feat: add active highlight to PdfViewer~~ feat: add focused highlight to PdfViewer Jan 12, 2022

fix: fix paging issue

9a1fe43

fukudasjp changed the title ~~feat: add focused highlight to PdfViewer~~ feat: add "active" state to highlight on PdfViewer Jan 12, 2022

jhpedemonte reviewed Jan 19, 2022

View reviewed changes

fukudasjp added 3 commits January 20, 2022 14:06

fix: fix highlight color contrast

8cb9c3f

Merge remote-tracking branch 'origin/master' into feat/active-pdf-hig…

8f03366

…hlight

fix: fix active highlight storybook

91569ff

fukudasjp added 2 commits January 20, 2022 16:58

fix: apply review comments

01d4ff7

chore: rollback format change in tsconfig.json

fb0c63e

fix: lgtm error

1e085b4

jhpedemonte approved these changes Jan 20, 2022

View reviewed changes

fukudasjp merged commit 35c473a into master Jan 21, 2022

fukudasjp deleted the feat/active-pdf-highlight branch January 21, 2022 04:15

Uh oh!

Conversation

fukudasjp commented Jan 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What do these changes do/fix?

How do you test/verify these changes?

Have you documented your changes (if necessary)?

Are there any breaking changes included in this pull request?

Uh oh!

jhpedemonte commented Jan 19, 2022

Uh oh!

jhpedemonte commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jhpedemonte left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fukudasjp commented Jan 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fukudasjp commented Jan 20, 2022

Uh oh!

lgtm-com Bot commented Jan 20, 2022

Uh oh!

fukudasjp commented Jan 20, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

fukudasjp commented Jan 11, 2022 •

edited

Loading

jhpedemonte commented Jan 19, 2022 •

edited

Loading

fukudasjp commented Jan 20, 2022 •

edited

Loading