-
Notifications
You must be signed in to change notification settings - Fork 5
CORE-1287: Port remaining book details page modules from JS to TS #2786
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
65d1634
to
584cdf5
Compare
94c0dff
to
852f593
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The first commit shows the JS->TS changes, and there are a few more changes in the 2nd commit.
852f593
to
2e6d867
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small file, so the changes were too much to show as changes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes are shown in the individual commits, but there are changes in each of the commits.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Even the first commit shows it as a delete-and-create. Sorry. :(
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, even the first commit has it as delete-and-create (the first commit was pretty much just Claude doing the types)
Converted all remaining JavaScript files in src/app/pages/details to TypeScript: Desktop view modules: - desktop-view.js → desktop-view.tsx - details-tab.js → details-tab.tsx - videos-tab.js → videos-tab.tsx - partners.js → partners.tsx - import-*.js → import-*.ts Phone view modules: - phone-view.js → phone-view.tsx - details-pane.js → details-pane.tsx - student-resources-pane.js → student-resources-pane.tsx - instructor-resources-pane.js → instructor-resources-pane.tsx All conversions follow TypeScript best practices: - Avoided using 'any' type - Preferred 'type' to 'interface' - Used type inference where possible - Inline type definitions for function parameters - Line lengths kept under 120 characters 🤖 Generated with [Claude Code](https://claude.com/claude-code) Port remaining book details page modules from JS to TS Converted all remaining JavaScript files in src/app/pages/details to TypeScript: Desktop view modules: - desktop-view.js → desktop-view.tsx - details-tab.js → details-tab.tsx - videos-tab.js → videos-tab.tsx - partners.js → partners.tsx - import-*.js → import-*.ts Phone view modules: - phone-view.js → phone-view.tsx - details-pane.js → details-pane.tsx - student-resources-pane.js → student-resources-pane.tsx - instructor-resources-pane.js → instructor-resources-pane.tsx All conversions follow TypeScript best practices: - Avoided using 'any' type - Preferred 'type' to 'interface' - Used type inference where possible - Inline type definitions for function parameters - Line lengths kept under 120 characters 🤖 Generated with [Claude Code](https://claude.com/claude-code) Revert to JS (loaders) Co-Authored-By: Claude <noreply@anthropic.com>
2e6d867
to
f0ec3e6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some minor comments about types
freeStuffStudent: StuffContent; | ||
freeStuffInstructor: StuffContent; | ||
videos: VideoContent[]; | ||
videos: [VideoContent[]] | never[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This type seems odd, but maybe only because the CMS data shape is odd?
<Option condition={model.cheggLink}> | ||
<a href={model.cheggLink} data-track="Chegg Reader"> | ||
<Option condition={model.cheggLink ?? false}> | ||
<a href={model.cheggLink as string} data-track="Chegg Reader"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type for this property seems confusing - it could be null, but not here? Do we need an assertString
helper?
link={webviewLink} | ||
variant="View online" | ||
warning={model.contentWarningText} | ||
warning={model.contentWarningText ?? undefined} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a big deal but I wonder if it would be nice to align the component types to the model so we don't have to do this
Summary
Converted all remaining JavaScript files in
src/app/pages/details
directory to TypeScript as requested in CORE-1287.Files Converted
Desktop view modules:
desktop-view.js
→desktop-view.tsx
details-tab.js
→details-tab.tsx
videos-tab.js
→videos-tab.tsx
partners.js
→partners.tsx
import-instructor-resource-tab.js
→import-instructor-resource-tab.ts
import-student-resource-tab.js
→import-student-resource-tab.ts
Phone view modules:
phone-view.js
→phone-view.tsx
details-pane.js
→details-pane.tsx
student-resources-pane.js
→student-resources-pane.tsx
instructor-resources-pane.js
→instructor-resources-pane.tsx
Guidelines Followed
All conversions adhere to the TypeScript best practices specified in the ticket:
any
typetype
tointerface
Changes Made
context.tsx
,resource-box-utils.tsx
, etc.)Test Plan
🤖 Generated with Claude Code