fix(site-builder): address Task 19 review — onChange convention, opt-out copy
Review findings on the Report an Issue modal: 1. Revert the description textarea from onInput back to onChange. The onInput swap sidestepped a real React value-tracker dedup gotcha (a test harness's raw `el.value = x` assignment looks like a no-op change to a controlled onChange input), but this codebase already has the correct fix for exactly that gotcha: bypass the tracker's patched setter via the native prototype descriptor, as HeadCodeModal.test.tsx, SiteDesignPanel.reset.test.tsx, shared-controls.test.tsx, and both MediaStylePanel.*.test.tsx already do. Rewrote typeDescription() in ReportIssueModal.test.tsx to use that idiom instead of bending the component to fit the test. 2. The include-contents opt-out copy said unchecking it still sends "your description, the page name and your browser details" but omitted console errors, which buildReportPayload always includes regardless of the checkbox and which can incidentally echo page content. Copy now lists console errors explicitly. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -50,10 +50,22 @@ function render(open = true) {
|
|||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|
||||||
|
// The textarea is a controlled input (`onChange`), so React's DOM value
|
||||||
|
// tracker patches its `value` setter to detect "did this really change".
|
||||||
|
// A plain `ta.value = text` assignment goes through that same patched
|
||||||
|
// setter, which updates the tracker's own record of "current value" as a
|
||||||
|
// side effect -- so by the time the dispatched 'input' event is handled,
|
||||||
|
// the tracker sees no difference and the synthetic onChange never fires.
|
||||||
|
// Bypassing the patched setter via the native prototype descriptor (same
|
||||||
|
// idiom as HeadCodeModal.test.tsx / SiteDesignPanel.reset.test.tsx /
|
||||||
|
// shared-controls.test.tsx / MediaStylePanel.*.test.tsx) sets the DOM value
|
||||||
|
// without touching the tracker, so the dispatched event is correctly seen
|
||||||
|
// as a real change.
|
||||||
function typeDescription(text: string) {
|
function typeDescription(text: string) {
|
||||||
const ta = document.querySelector('[data-testid="report-description"]') as HTMLTextAreaElement;
|
const ta = document.querySelector('[data-testid="report-description"]') as HTMLTextAreaElement;
|
||||||
act(() => {
|
act(() => {
|
||||||
ta.value = text;
|
const setter = Object.getOwnPropertyDescriptor(window.HTMLTextAreaElement.prototype, 'value')!.set!;
|
||||||
|
setter.call(ta, text);
|
||||||
ta.dispatchEvent(new Event('input', { bubbles: true }));
|
ta.dispatchEvent(new Event('input', { bubbles: true }));
|
||||||
});
|
});
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -210,16 +210,7 @@ export const ReportIssueModal: React.FC<ReportIssueModalProps> = ({ open, onClos
|
|||||||
<textarea
|
<textarea
|
||||||
data-testid="report-description"
|
data-testid="report-description"
|
||||||
value={description}
|
value={description}
|
||||||
// onInput rather than onChange: functionally identical for
|
onChange={(e) => setDescription(e.target.value)}
|
||||||
// real typing (both fire on every keystroke for a
|
|
||||||
// textarea), but onChange goes through React's
|
|
||||||
// value-tracker "did this really change" dedup, which a
|
|
||||||
// test harness driving the DOM via a raw `el.value =`
|
|
||||||
// assignment (rather than the native-setter-bypass trick)
|
|
||||||
// defeats -- the tracker sees its own just-written value
|
|
||||||
// and treats the dispatched 'input' event as a no-op.
|
|
||||||
// onInput is a plain passthrough with no such check.
|
|
||||||
onInput={(e) => setDescription((e.target as HTMLTextAreaElement).value)}
|
|
||||||
rows={5}
|
rows={5}
|
||||||
maxLength={MAX_DESCRIPTION_CHARS}
|
maxLength={MAX_DESCRIPTION_CHARS}
|
||||||
placeholder="What were you doing, and what did you expect to happen instead?"
|
placeholder="What were you doing, and what did you expect to happen instead?"
|
||||||
@@ -256,8 +247,8 @@ export const ReportIssueModal: React.FC<ReportIssueModalProps> = ({ open, onClos
|
|||||||
<span style={{ fontSize: 11, color: 'var(--color-text-muted)', lineHeight: 1.5 }}>
|
<span style={{ fontSize: 11, color: 'var(--color-text-muted)', lineHeight: 1.5 }}>
|
||||||
Include this page's contents to help debugging. This sends the text and
|
Include this page's contents to help debugging. This sends the text and
|
||||||
layout of the page you're editing along with your report. Uncheck it and
|
layout of the page you're editing along with your report. Uncheck it and
|
||||||
we'll still get your description, the page name and your browser details --
|
we'll still get your description, the page name, your browser details, and
|
||||||
but not the page's text or layout.
|
any recent console errors -- but not the page's text or layout.
|
||||||
</span>
|
</span>
|
||||||
</label>
|
</label>
|
||||||
|
|
||||||
|
|||||||
Reference in New Issue
Block a user