-
Notifications
You must be signed in to change notification settings - Fork 231
chore(compass-components): add typecheck to check, fix errors #7367
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
Conversation
customFocusTrapFallback={`#${innerContentTestId}`} | ||
setOpen={() => {}} | ||
trigger={({ onClick, ref, children }) => ( | ||
trigger={({ onClick, children }) => ( |
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 doesn't look right, ref needs to be passed to the trigger for the component to work (looking at the test, we just don't test the behavior that relies on this)
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.
Will push up some more tests shortly. Ran into something weird here I really don't know the cause of.
Making any of the tests in this file async
with a await setTimeout
like
await new Promise(resolve => setTimeout(resolve, 10));
(or our waitFor
helper) will cause the test to not report any results. I've tried with other tests in compass-components and they don't have the same behavior. It looks like it's something in the render of InteractivePopover that causes it. Maybe the focus trap? Not sure. Going to look at it a bit more, but I'll avoid spending too much time on it.
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.
Thanks for adding more specs! That's indeed weird, was looking through some other tests and what's weird is that some other components that use this one have async tests (at leat at a glance, maybe they're not hitting it) and those are passing 🤔
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.
Approved, but one change doesn't look quite right (it just doesn't currently has a meaningful effect on the tests because they are lacking)
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.
Pull Request Overview
This PR adds TypeScript type checking to the compass-components package as part of COMPASS-8170. The changes fix TypeScript errors that were revealed when enabling type checking by adding proper type annotations, handling potential null values with optional chaining, and adding missing required props.
Key changes:
- Added typecheck script to package.json and integrated it into the check command
- Added proper TypeScript type annotations for variables and function parameters
- Fixed potential null reference issues with optional chaining operators
- Added missing required props to component test fixtures
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.
Show a summary per file
File | Description |
---|---|
package.json | Added typecheck script and integrated it into the check command |
use-toast.spec.tsx | Extracted toast ID generation into a helper function |
use-sort.spec.ts | Added type casting to fix generic type issues |
loader.spec.tsx | Added type annotations for sinon spy variables |
list-editor.spec.tsx | Added type annotations for sinon spy variables |
interactive-popover.spec.tsx | Added type annotations and new test cases with proper cleanup |
guide-cue.spec.tsx | Added missing required props to component test |
guide-cue-service.spec.ts | Improved type safety by removing type casting |
file-picker-dialog.spec.tsx | Added type annotation for sinon spy variable |
document.spec.tsx | Added null checks and optional chaining for safer property access |
content-with-fallback.spec.tsx | Fixed array destructuring issue with HTMLCollection |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
packages/compass-components/src/components/content-with-fallback.spec.tsx
Show resolved
Hide resolved
@Anemy CI is green, so I'll merge because there's some changes to the tsconfig structure I'm planning to make and I don't want to cause conflicts for this patch. Thanks for doing this work! |
Part of COMPASS-8170
Related pr #7366