-
Notifications
You must be signed in to change notification settings - Fork 6
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, part 1 #2679
Conversation
6f78ff3
to
3fa5103
Compare
|
||
function FormWithAfterSubmit({ | ||
afterSubmit, | ||
responseId = 'form-response', | ||
children, | ||
onSubmit: firstOnSubmit, |
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 parameter was not used.
@@ -18,55 +24,45 @@ function Header({entry}) { | |||
); | |||
} | |||
|
|||
function PhoneBox({entry, closeAfterDelay}) { |
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.
It wasn't called with closeAfterDelay
))} | ||
</div> | ||
); | ||
} | ||
|
||
function Button({href, text, buttonClass, onClick}) { |
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.
Wasn't being called with onClick
); | ||
} | ||
|
||
describe('details/recommended-callout', () => { |
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.
Wrapped the tests in describe, which made it a major change (due to indenting everything)
Variant: (p: any) => React.JSX.Element, // eslint-disable-line @typescript-eslint/no-explicit-any | ||
p: object | ||
] { | ||
const {id, variant} = variantParams; |
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.
Do we want to use a rest object for the variantParams
to prevent id
and variant
being sent to the components?
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.
All of the components use id
and two of them use variant
. There's likely a better way to do the repetition-elimination I was doing here, but it hasn't become clear to me yet.
id?: string; | ||
variant?: string; | ||
}): [ | ||
Variant: (p: any) => React.JSX.Element, // eslint-disable-line @typescript-eslint/no-explicit-any |
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.
I wonder if a union type that uses ComponentProps would work here but it may not be worth the hassle
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.
I fiddled with it, but couldn't make it happy.
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.
Looks good but you may want to look at not sending all the variantParams if they aren't applicable to the variant components.
6022016
to
77051ac
Compare
[CORE-538]
Again, it's almost all adding/fixing types and tests.
pretttier
did some swapping of double quotes for single quotes and changed line wrapping.[CORE-538]: https://openstax.atlassian.net/browse/CORE-538?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ