-
Notifications
You must be signed in to change notification settings - Fork 5
Port components to ts 4 #2723
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?
Port components to ts 4 #2723
Conversation
() => new Intl.DisplayNames([language], {type: 'language'}), | ||
[language] | ||
const text = React.useMemo( | ||
() => (new Intl.DisplayNames([language], {type: 'language'})).of(locale), |
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.
We don't support browsers so old that Intl.DisplayNames isn't supported, so I took the fallback out.
@@ -41,7 +49,7 @@ export function Paginated({children, perPage=10}) { | |||
} | |||
useEffect(() => { | |||
if (pageChanged) { | |||
$.scrollTo(ref.current, 70); | |||
$.scrollTo(ref.current as HTMLDivElement, 70); |
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.
Don't you want to check for current being set before calling this? I checked the helper and it doesn't check for a null param so it seems like this could cause issues...
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.
As I understand it, when the useEffect
runs, the ref will be set, since it's set in the return value for the component. If I do a test for not-set, I won't be able to cover that branch with testing.
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 added an assertNotNull
helper function.
a8d3c58
to
9d295f0
Compare
CORE-857