Skip to content
This repository has been archived by the owner on Oct 7, 2022. It is now read-only.

Bugfix: StrokeWidth #72

Merged
merged 4 commits into from
Sep 12, 2022
Merged

Bugfix: StrokeWidth #72

merged 4 commits into from
Sep 12, 2022

Conversation

djmetzle
Copy link
Contributor

Fix a Fabric.js interface mismatch on our latest version.

Specifically, it appears master is trying to pass borderWidth to Fabric objects for their strokeWidth. This does not draw objects as expected. We see that

Here's a montage of the test images from master:
montage-master

⚠️ Notes:

  • The rectangle shape has been shifted slightly now. It appears the "corner" of the shape is now at the edge of the stroke, not centered in the corner as prior. This is difficult to describe, but it appears the position of the rectangle has changed to be about a half strokeWidth in the +X and +Y direction.
  • The white shadow/border is not being drawn as expected. Perhaps the mixin is not getting called the way we expect.

With these light fixes applied, we're actually drawing the shapes we expect, pretty close to our test oracle images:
montage-branch

qa_req 0 (another small step in the right direction here)

Ref: https://github.com/iFixit/ifixit/issues/43782

CC @iFixit/devops

We're setting up the `stroke` property, but Fabric wants us to use
`strokeWidth` here instead.
This is the object property we need for Fabric to actually widen out
marker strokes.
I'm not sure what this option/property is doing, but it's moving around
objects and lines in the image in very strange ways. Dropping this
property fixes positioning issues.
Fabric appears to rely on the `stroke` property for the color of
objects, not this color property. Drop it.
@djmetzle djmetzle added the bug label Sep 12, 2022
@djmetzle
Copy link
Contributor Author

Note, the slight rectangle shift, and lack of white shadow/border is highlighted well here:
image

Copy link
Member

@danielbeardsley danielbeardsley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Getting closer!

CR 👍

@jyee27
Copy link

jyee27 commented Sep 12, 2022

CR 🌵

@djmetzle djmetzle merged commit 9382752 into master Sep 12, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants