rusackas commented on PR #39461:
URL: https://github.com/apache/superset/pull/39461#issuecomment-4434414388
@sadpandajoe quick rollup of where things stand after this push:
**Addressed in `a396b654d`:**
- ✅ Removed the three `expect(true).toBe(true)` placeholders in
`SaveModal.test.tsx` rather than rewriting them inline — the FC-shaped versions
will live in a follow-up rather than as half-finished tests masquerading as
coverage.
- ✅ Added regression coverage for `CopyToClipboard` with string and number
`copyNode` to lock in the `isValidElement` guard you flagged.
**Behavior already in this PR, tests deferred** (per-thread replies above
explain the scaffolding cost):
- 📝 `history.replace({ saveAction })` — fix in `82bab7b03`, needs
`useHistory` spy
- 📝 `OUT_OF_TAB` URL strip — fix in `81123abec`, needs `useHistory.push` +
dashboard-create chain
- 📝 `onDashboardChange(null)` guard — fix in `81123abec`, needs
`mock-async-select` override or real `AsyncSelect`
- 📝 `partitionColumn` reset on datasource switch — fix in `8132732e3`, needs
popover/integration assert
- 📝 `fetchQueryResults` retry on late `resultsKey` — fix in `03cdd206c`,
needs `useQueryEditor` mock scaffolding
**Design feedback I'll address in follow-ups** (replies on individual
threads):
- 🔁 Re-add `memo()` to `SelectControl`, `AnnotationLayerControl`,
`AdhocFilter*`, `AdhocMetric*`, `FixedOrMetricControl` (lost `PureComponent`
skip)
- 🔁 Widen `ModalTrigger.modalTitle` to `string | ReactNode` so
`TextAreaControl` can pass back the full `<ControlHeader>` in the modal title
- 🔁 Stop rebinding `DatasourceEditor`'s keyboard shortcut on every keystroke
- 🔁 Investigate the `AnnotationLayer` double-fetch when `value` changes
- 🔁 Align `TextAreaControl` on the `{...restProps}` spread used by
`ViewportControl`
- 🔁 Drop unnecessary `useCallback`s in `AnnotationLayer` where no downstream
consumer is memoized
**Rebase notes:**
- Branch is rebased on current master (was a couple weeks behind).
- Dropped this PR's `CollectionControl` conversion — master already
converted it in #38563/#39862 and reverting would have lost the React-18 +
keyless-id-stability work. Net result: that file is back to master's version,
and the PR no longer covers it.
The scope-creep into follow-ups feels OK given this is the conversion PR,
not a perf/UX-fix PR — but happy to fold any of the above back in if you'd
rather block on them here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
