-
Notifications
You must be signed in to change notification settings - Fork 1
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
Update assignable launch #2253
Update assignable launch #2253
Conversation
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, just left a few questions to better my understanding.
|
||
jest.mock('react', () => { | ||
const react = (jest as any).requireActual('react'); | ||
return { ...react, useEffect: react.useLayoutEffect }; |
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 see that swapping this makes it synchronous, but I can't tell why at a glance. Is act not sufficient?
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.
its possible that newer versions of react or the testing library don't have this problem, but there are a few places in the rex code that do this. the react issue is here facebook/react#14050 (comment)
@@ -39,21 +42,60 @@ const StyledSectionTitle = styled.h2` | |||
`)} | |||
`; | |||
|
|||
declare const window: 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 don't notice any custom properties so it seems like you wouldn't need it this.... maybe prerendering related?
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 like an artifact of an earlier age
src/app/content/launchToken.ts
Outdated
export const pullToken = (window: Window) => { | ||
const searchParams = new URLSearchParams(window.location.search); | ||
|
||
const launchToken = searchParams.get('t') ?? 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.
The ??
branch seems pointless.
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.
you're right, the conditional below catches it either way
https://openstax.atlassian.net/browse/DISCO-14
adds token parsing for assignable integration and textSize support from the assignable nav. integrates with changes from https://github.com/openstax/assignments/compare/main...text-resizer?expand=1