-
Notifications
You must be signed in to change notification settings - Fork 86
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
refactor(simple-select): convert enzyme tests to RTL #6992
Conversation
34f680f
to
5575b47
Compare
@@ -231,16 +229,16 @@ export const SimpleSelect = React.forwardRef< | |||
|
|||
filterText.current = newVal; | |||
selectValueStartingWithText(newVal); | |||
clearTimeout(filterTimer.current as TimerId); | |||
window.clearTimeout(filterTimer.current); |
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.
Comment (non-blocking): it might be worth looking if we can come up with something that allows us to set these globally like below, happy for that to be a future bit of work rather than part of this PR etc
global.setTimeout = function (callback, delay) {
return window.setTimeout(callback, delay);
};
global.setInterval = function (callback, delay) {
return window.setInterval(callback, delay);
};
global.clearTimeout = function (id) {
return window.clearTimeout(id);
};
global.clearInterval = function (id) {
return window.clearInterval(id);
};
@@ -331,6 +331,7 @@ export const SimpleSelect = React.forwardRef< | |||
useEffect(() => { | |||
return function cleanup() { | |||
window.clearTimeout(filterTimer.current); | |||
window.clearTimeout(focusTimer.current); |
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.
suggestion (non-blocking): you could probably fold this change into the commit before but it's not critical etc
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'll make sure to do this before release 👍🏼
<Option id="red" value="red" text="red" /> | ||
</SelectListWithInput> | ||
); | ||
it.each(["top", "bottom", "left", "right"] as const)( |
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.
comment: FYI these are the tests I've changed as part of #6991, I suspect this PR will get merged before it but just in case wanted to make you aware where the conflict might arise
expect(screen.getByRole("textbox")).toBeInTheDocument(); | ||
}); | ||
|
||
test("applies transparent background and no border to input, when transparent prop is true", () => { |
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.
question/suggestion(non-blocking): Is this required for coverage? If not, this could be moved to a chromatic snapshot. If it is required, a comment above saying it is like we have elsewhere would be ideal.
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.
Yeah this is for satisfying coverage, I'll leave a comment confirming this 👍🏼
5575b47
to
cbed1af
Compare
…improve test coverage reporting
cbed1af
to
18c9490
Compare
eb02df8
to
bbbc891
Compare
🎉 This PR is included in version 143.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Proposed behaviour
Current behaviour
Checklist
d.ts
file added or updated if required