feat: convert AlertModal to TypeScript#4142
feat: convert AlertModal to TypeScript#4142brian-smith-tcril wants to merge 3 commits intoopenedx:release-23.xfrom
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Deploy Preview for paragon-openedx-v23 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
The original TypeScript can't express "required when another prop is falsy" without overloads or a discriminated union, which felt like overkill for what was just a warning. Making it optional with a default of |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-23.x #4142 +/- ##
================================================
+ Coverage 94.39% 94.42% +0.03%
================================================
Files 242 242
Lines 4296 4303 +7
Branches 1020 1025 +5
================================================
+ Hits 4055 4063 +8
+ Misses 237 232 -5
- Partials 4 8 +4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Use React.ComponentProps<typeof ModalDialog> + Omit to restore the
original ...props passthrough to ModalDialog
- Redeclare onClose as optional (AlertModal defaults it to () => {})
- Redeclare isOverflowVisible as optional with default false (was absent
from original propTypes, undefined is falsy)
- Redeclare variant without 'dark' (intentionally excluded in original)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
| // - isOverflowVisible: required in ModalDialog but was absent from AlertModal's propTypes; | ||
| // undefined is falsy so defaulting to false matches the previous behavior |
There was a problem hiding this comment.
I think defaulting to false here (as opposed to making this a required prop) is the right call for this PR (see #4142 (comment))
Cover the onClose, footerNode, and isOverflowVisible default parameter branches, and the no-footer render path, to restore 100% coverage. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
3d9bdd8 to
dea6dda
Compare
Part of #3739 (AlertModal item)
Summary
Converts
AlertModalfrom.jsxto.tsx:propTypesanddefaultPropsAlertModalPropsinterface with JSDoc commentssrc/index.tsto moveAlertModalinto the typed exports sectionTesting
npm run type-checkpasses cleanAlertModaltests passLog
paragon JSX→TSX conversion (issue #3739)
Background
openedx/paragon is a React component library used across Open edX MFEs. Issue #3739 is a tracking issue to convert all remaining
.jsxcomponents to.tsx— eliminating deprecatedpropTypes/defaultPropsAPIs, clearing the path to React 19, and giving consumers build-time type errors.Components are sorted by usage count.
AlertModalhas ~30 uses and was self-assigned by @brian-smith-tcril. The work involves converting one component per PR (the project asks for 1–3 components per PR to keep review manageable).We started this session by: reading the issue, identifying the component, cloning the fork, and following the patterns established by earlier merged PRs (Spinner #3714, Badge/Breadcrumb #3807, PageBanner/Menu #3831, etc.).
Repo: /home/bsmith/code/claude/paragon
Branch: alertmodal-to-ts (off release-23.x)
Remotes: origin = SSH (brian-smith-tcril fork), upstream = https (openedx/paragon, no push)
Shell setup
nvm is not on PATH in Bash tool sessions — must source it first:
Conversion pattern
.jsx→.tsx, removePropTypesanddefaultPropsinterface <Component>Props { ... }with JSDoc comments from propTypesdefaultPropsinto function parameter destructuringPropTypes.node→React.ReactNode,PropTypes.elementType→React.ComponentType(notReact.ElementType— Icon's.d.tsexplains why).jsxfilesrc/index.ts: move export from "no types" section (remove// @ts-ignore) into "has types" section (preserve alphabetical order)Example conversions to reference
src/PageBanner/index.tsxsrc/Breadcrumb/index.tsxsrc/Badge/index.tsx(uses forwardRef + Bootstrap base)src/Modal/ModalDialog.tsx(already converted, good reference for Modal-family props)Known issue: pre-commit hook crashes on Node 24
lint-staged uses
npx eslint(seepackage.jsonlint-stagedconfig) while CI usesnpm run eslint→eslint ... .. On Node 24,npx eslintcrashes with:This is a Node 24 incompatibility in
es-iterator-helpers(used byeslint-plugin-react). The fullnpm run eslintpasses clean. Workaround:git commit --no-verify. Issue filed: #4141AlertModal specifics
src/Modal/AlertModal.tsx(was.jsx)ModalDialog— all ModalDialog props passed explicitly (no...propsspread to ModalDialog)onCloseis optional in AlertModal (defaults to() => {}), required in ModalDialog — default handles thisisOverflowVisiblenot in original propTypes but needed by ModalDialog; added as optional, defaults tofalseicontype:React.ComponentType(notReact.ElementType) persrc/Icon/index.d.tsalertmodal-to-tsnotfeat/alertmodal-to-ts)--no-verify(pre-commit hook broken on Node 24, see above)🤖 Generated with Claude Code