Skip to content
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

Port details page to ts 2 #2680

Merged
merged 7 commits into from
Nov 11, 2024
Merged

Port details page to ts 2 #2680

merged 7 commits into from
Nov 11, 2024

Conversation

RoyEJohnson
Copy link
Contributor

@RoyEJohnson RoyEJohnson commented Nov 4, 2024

CORE-592
This builds on #2679 which should be merged first.

@RoyEJohnson RoyEJohnson force-pushed the port-details-page-to-ts-2 branch 2 times, most recently from 30cca47 to 6ae7c3e Compare November 8, 2024 15:53
@@ -5,11 +5,9 @@ import memoize from 'lodash/memoize';
export function bookToc(slug: string) {
return cmsFetch(slug)
.then((bi) => {
const isRex = true;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All books are Rex now, so I removed the CNX fetching stuff.

}

export default function AuthorsSection() {
const {authors, title} = useDetailsContext();
Copy link
Contributor Author

@RoyEJohnson RoyEJohnson Nov 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

had been passing model around, but it's just the info from the Context. polish is derived from it.

const title = model.title;
const blurb = model.errataContent;
export function ErrataContents() {
const {title, errataContent: blurb, bookState} = useDetailsContext();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, model and polish didn't need to be parameters.


type LeftContentModelType = {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was a different definition of what should have been ResourceModel


const settings = window.SETTINGS;

export default function ResourceBox({model}) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I split the components out into resource-box.tsx, and left the utilities in here, renaming it resource-box-utils.js It will go to TS in the next PR.

@@ -16,12 +17,10 @@ type VideoResourceBoxModelType = {
resourceCategory: string;
};

type BoxModel = { heading: string };
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another that should have just been ResourceModel

onClick={() => toggle()}
onKeyDown={treatSpaceOrEnterAsClick}
aria-controls="toc-drawer"
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A little accessibility

{viewsUsed.desktop && <JITLoad importFn={importDesktopView} />}
</div>
{
viewsUsed.phone &&
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved the condition to the outside to control the entire view.

@RoyEJohnson RoyEJohnson requested a review from jivey November 8, 2024 19:27
Copy link
Member

@jivey jivey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor concern about the deleted test and some questions.

src/app/pages/details/common/hooks.tsx Outdated Show resolved Hide resolved
src/app/pages/details/common/hooks.tsx Outdated Show resolved Hide resolved
test/src/pages/details/dual-view.test.tsx Outdated Show resolved Hide resolved
test/src/pages/details/details.test.tsx Outdated Show resolved Hide resolved
@RoyEJohnson RoyEJohnson force-pushed the port-details-page-to-ts-2 branch from a0c23a1 to a72bef4 Compare November 11, 2024 13:37
@RoyEJohnson RoyEJohnson requested a review from jivey November 11, 2024 15:15
@RoyEJohnson RoyEJohnson merged commit 86699ce into main Nov 11, 2024
1 check passed
@RoyEJohnson RoyEJohnson deleted the port-details-page-to-ts-2 branch November 11, 2024 16:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants