Feature/new react sketch pad component by KevinBusch · Pull Request #45 · rsm-hcd/AndcultureCode.JavaScript.React.Components

KevinBusch · 2020-08-24T19:53:22Z

#42
NOTE:

Related GitHub issue(s) linked in PR description
Destination branch merged, built and tested with your changes
Code formatted and follows best practices and patterns
Code builds cleanly (no additional warnings or errors)
Manually tested
Automated tests are passing
[-] No decreases in automated test coverage (will back fill tests after review)
[-] Documentation updated (readme, docs, comments, etc.)
Localization: No hard-coded error messages in code files (minimally in string constants)
New component and/ or properties? Make sure to update storybook

…Code.JavaScript.React.Components into feature/new-react-sketch-pad-component

… functions

…ndall

codecov · 2020-08-24T19:58:22Z

Codecov Report

Merging #45 into master will decrease coverage by 22.79%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##           master      #45       +/-   ##
===========================================
- Coverage   41.64%   18.84%   -22.80%     
===========================================
  Files          41       51       +10     
  Lines         401      886      +485     
  Branches      117      202       +85     
===========================================
  Hits          167      167               
- Misses        224      709      +485     
  Partials       10       10

Impacted Files	Coverage Δ
src/atoms/forms/canvas-sketch/canvas-sketch.ts	`0.00% <0.00%> (ø)`
.../atoms/forms/canvas-sketch/react-canvas-sketch.tsx	`0.00% <0.00%> (ø)`
...forms/canvas-sketch/tools/base-canvas-draw-tool.ts	`0.00% <0.00%> (ø)`
...toms/forms/canvas-sketch/tools/base-canvas-tool.ts	`0.00% <0.00%> (ø)`
...oms/forms/canvas-sketch/tools/image-canvas-tool.ts	`0.00% <0.00%> (ø)`
...forms/canvas-sketch/tools/line-canvas-draw-tool.ts	`0.00% <0.00%> (ø)`
...atoms/forms/canvas-sketch/tools/pan-canvas-tool.ts	`0.00% <0.00%> (ø)`
...rms/canvas-sketch/tools/pencil-canvas-draw-tool.ts	`0.00% <0.00%> (ø)`
.../atoms/forms/canvas-sketch/utils/position-utils.ts	`0.00% <0.00%> (ø)`
src/utilities/core-utils.ts	`0.00% <0.00%> (ø)`
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1c9c686...c02142e. Read the comment docs.

brandongregoryscott

Still looking through this, but I left some initial feedback. I might be missing some context around how these components interact, but the relationship between react-canvas-sketch.tsx and canvas-sketch.tsx wasn't very easy to follow as a reader.

brandongregoryscott · 2020-08-26T23:04:59Z

+        this._redrawSketch();
+    }
+
+    public redrawBackgroundImageUsing(backgroundImageUrl: string): void {


Nitpick - alphabetization of methods within a region

brandongregoryscott · 2020-08-26T23:07:00Z

+            return;
+        }
+        if (newCurrentObjectIndex < -1) {
+            console.error(`Cannot set new object index below -1.  -1 should be used when nothing is to be drawn`);


Should these errors be shown in a production environment? What might cause data to be set in this state?

If they should only be shown (or will only be useful) in a development environment, we do have an EnvironmentUtils to check for a dev environment & run a function only in development.

Nice to know about the EnvironmentUtils, thanks!

I think these errors should show. These indicate improper use of this library. In this example, the consumer was attempting to set the index of a stack (array of pen/line strokes) below -1 which, to this library in an illegitimate value. -1 represents that nothing in the stack is selected and therefore nothing should be drawn. [0-n] represents where in the stack the pen/line strokes should be drawn.

brandongregoryscott · 2020-08-26T23:08:09Z

+            return;
+        }
+
+        if (this.selectedTool) {


Prefer explicit non-null check over a truthy check

brandongregoryscott · 2020-08-26T23:12:36Z

+            return;
+        }
+        this.selectedTool = selectedTool;
+        if (this.selectedTool) {


Same here ☝️ Prefer explicit non-null check

brandongregoryscott · 2020-08-26T23:13:45Z

+        this._clearCanvas(this._config.sketchCanvas, this._sketchContext);
+    }
+
+    private _clearBackgroundImageCanvas(): void {


Nitpick - alphabetization of methods within a region

brandongregoryscott · 2020-08-26T23:24:54Z

@@ -0,0 +1,397 @@
+import { ImageSettings, ImageCanvasTool } from "./tools/image-canvas-tool";


Overall, I would recommend adding some JSDoc commentary around some of these methods and how they interact.

👍 Will do

brandongregoryscott · 2020-08-26T23:28:38Z

+}
+
+export interface ReactCanvasSketchProps {
+    backgroundImageUrl: string;


Would recommend adding some JSDoc commentary around these props (same goes for the component above)

brandongregoryscott · 2020-08-26T23:37:06Z

+    // set up effects
+    useEffect(() => {
+        console.log("useEffect: initializing");
+        if (!isInitialized) {


Would recommend short circuiting if isInitialized is true

brandongregoryscott · 2020-08-26T23:38:12Z

+            return;
+        }
+        canvasSketch.redrawSketchAt(props.value.objects, props.value.currentObjectIndex);
+    }, [props.redrawIncrement, canvasSketch, props.value.objects, props.value.currentObjectIndex]);


Not really sure what this useEffect is accomplishing vs. the one right above it - might be some intricacies in the dependency list that I'm missing, but could use some additional explanation

This effect hook is triggered anytime the renderer of <ReactCanvasSketch/> passes in a different redrawIncrement integer where as the other effect hook doesn't have a dependency on redrawIncrement. This is a way I've allowed high level components control their rendered subordinates by way of props instead of tracking refs which I avoid as much as possible.

brandongregoryscott · 2020-08-27T00:02:27Z

+        props.onAddedStroke,
+        props.toolColor,
+        props.toolWidth,
+        props.value.currentObjectIndex,


Just a note, we've experienced some weirdness with putting deeply nested properties in a dependency list for a hook. If it's working for you, great - just something that I have avoided due to some unexpected behavior.

Honestly I wasn't planning on adding all of these dependencies but Typescript is configured to complain about it because they're all being used inside the hook. In it's current implementation, all of these dependencies are necessary for configuring the CanvasSketch object at initialization, although I suppose setSelectedTool(), setToolColor(), setToolWidth(), and setOnAddedToolStroke() are probably not needed here since they're already being done in different hooks. I'll remove them and validate that. I'm all ears if you have any suggestions besides that.

You could probably listen for props and then destructure the properties you care about inside of the hook - or, destructure the properties you care about from outside the hook, and then add those in the dependency list instead.

Take a look at this thread: facebook/react#16265

@brandongregoryscott That thread was helpful, thanks! I updated the code since it wasn't optimized for initialization of the canvas sketch so I got around it that way. Nice to know to destructure in advance though according to best practive.

KevinBusch · 2020-08-27T11:28:02Z

Still looking through this, but I left some initial feedback. I might be missing some context around how these components interact, but the relationship between react-canvas-sketch.tsx and canvas-sketch.tsx wasn't very easy to follow as a reader.

canvas-sketch.tsx isn't a component at all. It's actually canvas-sketch.ts and it's really a separate javascript library which is responsible for tracking to canvas elements (1 for a background image and 1 for drawing) provided to it, binding the selected tool's interactions with the drawing/sketch canvas, and bubbling drawing interactions back to the consumer.

react-canvas-sketch.tsx is responsible for rendering the two canvases and instantiating/maintining an instance of CanvasSketch using those two canvases within it's own state. The hooks are meant to trigger the various public API methods of CanvasSketch to do things like switch color, width, selected tool, etc...

I'll add this documentation when I get some time. I probably should have done this before passing it off.

…to call it when redraw incrementer changes

…every time effectively clearing out the canvas

KevinBusch · 2020-08-31T14:40:47Z

@brandongregoryscott At this point, what else should I change / add to get this to a point where it's approved and merged?

brandongregoryscott · 2020-08-31T21:05:55Z

@KevinBusch Took a look at your changes since the last time I looked - thanks for adding all the commentary and cleaning up. I think there are few missed recommended refactors (mostly around nesting/short-circuiting), but I think this is good to go for now 👍

KevinBusch · 2020-09-01T11:22:44Z

@brandongregoryscott I think I updated all the places you commented on including the nesting/short-circuiting suggested changes. I'd be happy to update wherever I missed. Let me know.

brandongregoryscott · 2020-09-02T13:00:12Z

@KevinBusch A few of the missed places I'm seeing:

https://github.com/AndcultureCode/AndcultureCode.JavaScript.React.Components/pull/45/files/5c5a20694d73452c00f08f5c20ee26ac7eaedc96#diff-76ffe4d4b843b64c527255e1cbd2b550R276

https://github.com/AndcultureCode/AndcultureCode.JavaScript.React.Components/pull/45/files/5c5a20694d73452c00f08f5c20ee26ac7eaedc96#diff-76ffe4d4b843b64c527255e1cbd2b550R299

https://github.com/AndcultureCode/AndcultureCode.JavaScript.React.Components/pull/45/files/5c5a20694d73452c00f08f5c20ee26ac7eaedc96#diff-76ffe4d4b843b64c527255e1cbd2b550R332

Not a big deal, but would probably help a reader follow the logic paths. Feel free to PR it in if you have time 👍

brandongregoryscott · 2021-05-17T12:39:27Z

@all-contributors add @KevinBusch for code, doc

allcontributors · 2021-05-17T12:39:35Z

@brandongregoryscott

I've put up a pull request to add @KevinBusch! 🎉

Kevin Busch added 9 commits August 13, 2020 13:31

This is a test

e0a28b2

Initial add of all the sketchpad files

dda27fa

Merge branch 'master' of https://github.com/AndcultureCode/Andculture…

8c6c85a

…Code.JavaScript.React.Components into feature/new-react-sketch-pad-component

Added a core utils object with bindAll in order to bind to all object…

6daaaf0

… functions

Removed unnecessary console logs. Updated canvas sketch's coreutil.bi…

5c9b11c

…ndall

Cleanup

e92ac0f

Added border feature

52c1b88

Filled in some defaults for storybook

a9089e4

Updated pencil tool to capture data in more semantic way

5c5a206

KevinBusch assigned wintondeshong and brandongregoryscott Aug 24, 2020

brandongregoryscott reviewed Aug 27, 2020

View reviewed changes

Kevin Busch added 7 commits August 31, 2020 08:29

Added JSDocs

20c0337

Added JSDocs to the sketchpad component

231294c

Refactored effect hooks to track changes on a more granular scale

154ba27

Made canvas sketch's redrawSketch public and updated react component …

d9aeaef

…to call it when redraw incrementer changes

Removed unnecessary console log

88b9983

Moved props in story to local variables so they aren't reinitialized …

e0def85

…every time effectively clearing out the canvas

Removed console logs

c02142e

brandongregoryscott approved these changes Aug 31, 2020

View reviewed changes

brandongregoryscott merged commit fff7511 into rsm-hcd:master Aug 31, 2020

allcontributors Bot mentioned this pull request May 17, 2021

docs: add KevinBusch as a contributor #120

Merged

		@@ -0,0 +1,397 @@
		import { ImageSettings, ImageCanvasTool } from "./tools/image-canvas-tool";

Conversation

KevinBusch commented Aug 24, 2020

Uh oh!

codecov Bot commented Aug 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

brandongregoryscott 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!

Choose a reason for hiding this comment

Uh oh!

KevinBusch commented Aug 27, 2020

Uh oh!

KevinBusch commented Aug 31, 2020

Uh oh!

brandongregoryscott commented Aug 31, 2020

Uh oh!

KevinBusch commented Sep 1, 2020

Uh oh!

brandongregoryscott commented Sep 2, 2020

Uh oh!

brandongregoryscott commented May 17, 2021

Uh oh!

allcontributors Bot commented May 17, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov Bot commented Aug 24, 2020 •

edited

Loading