[WC-3297] Signature web#2138
Conversation
gjulivan
force-pushed
the
signature-web
branch
8 times, most recently
from
1edd154 to
c439717
Compare
There was a problem hiding this comment.
don't we have the same structure as in combobox?
There was a problem hiding this comment.
We can keep this one, but let's remove:
- Architecture seciton
- Key Files section
- Properties - they will be later added as props.md
gjulivan
force-pushed
the
signature-web
branch
2 times, most recently
from
c154fa9 to
e9fee3d
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -0,0 +1,44 @@ | |||
| import classNames from "classnames"; | |||
| import { ReactElement } from "react"; | |||
| import { useSignaturePad } from "src/utils/useSignaturePad"; | |||
There was a problem hiding this comment.
🔶 Medium — Non-absolute import path will fail in build
import { useSignaturePad } from "src/utils/useSignaturePad" uses a bare src/ prefix. Rollup and TypeScript moduleResolution: "node" resolve this against node_modules, not the package root. The build will error unless src is aliased — but there is no such alias in tsconfig.json or rollup.config.mjs.
Fix: Change to a relative import:
| import { useSignaturePad } from "src/utils/useSignaturePad"; | |
| import { useSignaturePad } from "../utils/useSignaturePad"; |
| if (imageDataUrl) { | ||
| const customFileName = fileName?.value || Utils.generateFileName("signature"); | ||
| imageSource.setValue(Utils.convertUrlToBlob(imageDataUrl, customFileName)); |
There was a problem hiding this comment.
🔶 Medium — imageSource.setValue() called even when attribute is read-only
imageSource.setValue(...) is called unconditionally when imageDataUrl is truthy. When imageSource.readOnly === true the runtime silently discards the value.
Fix: Add !imageSource.readOnly guard:
| if (imageDataUrl) { | |
| const customFileName = fileName?.value || Utils.generateFileName("signature"); | |
| imageSource.setValue(Utils.convertUrlToBlob(imageDataUrl, customFileName)); | |
| if (imageDataUrl && !imageSource.readOnly) { | |
| imageSource.setValue(convertUrlToBlob(imageDataUrl, getFileName(imageSource))); | |
| } |
| const handleSignEnd = useCallback(() => { | ||
| const imageDataUrl = signaturePadRef.current?.toDataURL(); | ||
|
|
||
| if (hasSignatureAttribute) { | ||
| hasSignatureAttribute.setValue(!signaturePadRef.current?.isEmpty()); | ||
| } | ||
| if (imageDataUrl && onSignEnd) { | ||
| onSignEnd(imageDataUrl); | ||
| } | ||
| }, [hasSignatureAttribute, onSignEnd]); |
There was a problem hiding this comment.
🔶 Medium — onSignEnd missing from useCallback dependency array
onSignEnd is used inside handleSignEnd but is absent from useCallback's deps. When the parent re-renders with a new onSignEnd reference, the memoised callback retains the old one — stale closure bug.
Fix: Add onSignEnd to the dependency array on the closing line of this useCallback.
| useEffect(() => { | ||
| if (canvasRef.current) { | ||
| // only instantiate when all data is loaded properly to avoid unnecessary re-instantiations | ||
| const canInstantiateSignaturePad = | ||
| signaturePadRef.current === null && | ||
| (imageSource?.status === "available" ? imageSource.value?.uri : imageSource.status === "unavailable"); | ||
| if (canInstantiateSignaturePad && !isSignatureInitialized.current) { | ||
| signaturePadRef.current = new SignaturePad(canvasRef.current, { | ||
| penColor, | ||
| ...signaturePadOptions | ||
| }); | ||
| signaturePadRef.current.addEventListener("endStroke", handleSignEnd); | ||
| if (readOnly) { | ||
| signaturePadRef.current?.off(); | ||
| } | ||
| isSignatureInitialized.current = true; | ||
| } | ||
| } | ||
| }, [handleSignEnd, penColor, readOnly, signaturePadOptions, imageSource, hasSignatureAttribute]); |
There was a problem hiding this comment.
🔶 Medium — SignaturePad event listener is never removed on unmount
The initialization useEffect attaches pad.addEventListener("endStroke", handleSignEnd) but returns no cleanup function. On unmount the listener leaks and handleSignEnd fires against stale/deallocated props.
Fix: Return a cleanup function from this effect:
return () => {
pad.removeEventListener("endStroke", handleSignEnd);
pad.off();
};
| @@ -0,0 +1,63 @@ | |||
| import "@testing-library/jest-dom"; | |||
There was a problem hiding this comment.
🔶 Medium — Insufficient unit test coverage
The test suite mocks out SignatureComponent entirely and only verifies the thin container renders and forwards props. The actual widget logic — sign-end callback, canExecute guard, read-only prevention, clear-on-attribute-change, resize — has zero coverage, despite the PR description claiming "comprehensive unit tests".
Fix: Add tests directly against SignatureComponent using @mendix/widget-plugin-test-utils builders (EditableValueBuilder, actionValue()). Key cases to cover:
canExecute: falsemust not callexecutehasSignatureAttributetransitioningtrue → falseclears the canvas- Read-only
imageSourcepreventssetValue
| @@ -0,0 +1,44 @@ | |||
| // eslint-disable-next-line @typescript-eslint/no-extraneous-class | |||
There was a problem hiding this comment.
The @typescript-eslint/no-extraneous-class suppression on this file exists only to allow an all-static class. Plain exported functions are idiomatic TypeScript, tree-shakable, and eliminate the need for this suppression comment entirely.
| > | ||
| <Grid | ||
| gridBorderColor={gridBorderColor || "#000000"} | ||
| gridBorderWidth={gridBorderWidth || 50} |
There was a problem hiding this comment.
gridBorderWidth fallback is 50px instead of 1px
gridBorderWidth || 50 uses ||, so an intentional value of 0 falls back to 50 (50px border). The XML default is 1. Use nullish coalescing to only fall back on null/undefined:
| gridBorderWidth={gridBorderWidth || 50} | |
| gridBorderWidth={gridBorderWidth ?? 1} |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
AI Code Review
What was reviewed
Skipped (out of scope): Findings🔶 Medium —
|
3 participants
Signature Widget v2.0 - Custom Widget to Pluggable Widget Migration
Summary
Complete rewrite of the Signature widget, migrating from the legacy Custom Widget architecture (v1.0.8) to the modern
Pluggable Widget API (v2.0.0). This is a major architectural change that brings the widget in line with Mendix's current
standards and provides better Studio Pro integration, improved maintainability, and enhanced functionality.
🚨 Breaking Changes
com.mendix.widget.custom.signature.Signature→com.mendix.widget.web.signature.Signature✨ What's New
Added Features
fileNameproperty (textTemplate) for customizing saved file names@mendix/widget-plugin-component-kitValidationAlertTechnical Improvements
@mendix/pluggable-widgets-toolsreplacing Webpack🔄 Migration Notes
For Developers
packages/pluggableWidgets/signature-web/(new) vspackages/customWidgets/signature-web/(legacy)pnpm run build,pnpm run dev,pnpm run testpackages/customWidgets/for referenceFor App Builders
📋 Preserved Functionality
All v1.x features are maintained:
signature_padv5.1.3🧪 Testing Completed
pnpm run test)pnpm run lint)pnpm run build)