attach multiple refs to canvas element

[mkong0216]

Previously we attached a ref directly to the <SegmentCanvas />component but after updating our dependencies (specifically React), a warning message is now displayed in the console as shown below:

After looking into the <SegmentCanvas /> I noticed that <SegmentCanvas /> also has attached a ref to its canvas element. In order to attach two refs to the same canvas element, I have forwarded the ref from the <Segment /> component as a prop called forwardRef and called a function to attach both this.props.forwardRef and this.canvasEl to the canvas element similar to the solution discussed here.

Another option that I am thinking is to only have one ref from the <Segment /> component be passed into <SegmentCanvas /> and attached to the canvas element. Instead of creating a new ref, this.canvasEl, in <SegmentCanvas /> we could reuse this.props.forwardRef (or rename it to this.props.canvasEl) to do what is needed in <SegmentCanvas /> which is to draw the segment. This way we could use the React.forwardRef as shown here?

@louh Let me know which way sounds better. I think both of these solutions will remove the warning displayed in console.

[louh]

Thanks for looking into this! It seems to me that using React.forwardRef() as an HOC wrapper around <SegmentCanvas> is more idiomatic React code, but what happens when other components like <SegmentForPalette> don't need to create their own ref and forward it? Will <Segment> still generate its own ref for internal use?

[mkong0216]

I was worried that attaching refs like I am doing in this PR was not the "right"/idiomatic way in React so I was trying to figure out another solution, but you are right, <SegmentForPalette /> wouldn't be passing a ref which wouldn't work when using forwardRef.

[louh]

Here's another food for thought, what happens if we pass updatePerspective all the way down to SegmentCanvas?

[codecov-io]

Codecov Report

Merging #1290 into master will decrease coverage by 0.12%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master    #1290      +/-   ##
==========================================
- Coverage   33.24%   33.12%   -0.13%     
==========================================
  Files         219      219              
  Lines        8413     8632     +219     
  Branches     1841     1938      +97     
==========================================
+ Hits         2797     2859      +62     
- Misses       5061     5181     +120     
- Partials      555      592      +37

Impacted Files	Coverage Δ
assets/scripts/segments/Segment.jsx	90.9% <ø> (+1.25%)	⬆️
assets/scripts/segments/SegmentCanvas.jsx	83.78% <50%> (-4.1%)	⬇️
assets/scripts/segments/segment-dict.js	0% <0%> (ø)	⬆️
assets/scripts/app/StreetView.jsx	0.52% <0%> (+0.52%)	⬆️
assets/scripts/segments/view.js	56.85% <0%> (+1.36%)	⬆️
assets/scripts/segments/info.js	97.29% <0%> (+1.46%)	⬆️

---

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 c14bd24...fb10552. Read the comment docs.

[mkong0216]

Just did what you recommended! I had to pass this.state.switchSegments into <SegmentCanvas /> as well so that <SegmentCanvas /> knows when to call updatePerspective.

[louh]

Great! I'm glad that wasn't too hard and it also cleans up the code. I was just looking at the switchSegments prop -- do we really need it?

Here's my train of thought:

• In <Segment>, the state.switchSegments is set to true whenever the segment's variantString is different from before.
• We do pass variantString directly to <SegmentCanvas> already.
• Instead of checking whether the switchSegments prop changes, why not just check whether the variantString prop changes?

One last comment: since updatePerspective isn't always passed in, it should be checked if it's present, or be given a default no-op function.

[mkong0216]

So turns out that variantString only changes for the old segment canvas since when we are switching variants, a new <SegmentCanvas /> is rendered hence calling updatePerspective in componentDidMount.

UPDATE:
Does setting the perspective before a segment is being switched away affect anything negatively? I called updatePerspective in componentDidMount because variantString does not change for the new segment canvas but maybe I should be thinking of another way?

[louh]

So I think this is fine. We've solved the original problem (the refs warning) in a way that keeps our code clean. I'm very pleased with the result.

I did a little bit of reflecting on how and when we calculate perspectives, and if #1145 is still an extant bug (I assume it is, but haven't investigated further) then there is a problem that runs a little deeper than the ref issue. In which case we may need to reconsider when it's being called anyway, and I'm happy to deal with later - as it's out of scope for this PR.

Merging now!