-
Notifications
You must be signed in to change notification settings - Fork 12
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Start of repo and AttachResult component #1
Conversation
Added AttachResult component and tests.
detach: (queryResult: IQueryResult) => Promise<void>; | ||
/** Optional function to check the initial state of the component. */ | ||
isAttached?: (queryResult: IQueryResult) => Promise<boolean>; | ||
} |
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.
If you ever want to provide an option for tooltip positioning, instead of it always being at the bottom, for the Quickview component we use PopperJs
https://github.com/coveo/search-ui/blob/master/src/ui/Quickview/Quickview.ts
tests/ui/AttachResult.spec.ts
Outdated
it("should be attached when isAttached returns true", (done) => { | ||
isAttachedSpy.and.returnValue(Promise.resolve(true)); | ||
attachResult = Mock.optionsResultComponentSetup(AttachResult, { isAttached: faker.isAttachedMock }, fakeResult); | ||
setTimeout(() => { |
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.
Seems strange that these setTimeout
are needed?
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.
To simulate the back and forth of a server call?
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 was having a hard time trying to make the test work, for some reason I cannot call done()
in the spy .and.callFake
, the test kept timeouting.. do you have an idea why?
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.
Hmm, I've run into problems when using promises with the jest framework. There I solve it by moving the async method into the beforeEach
. If the promise is the issue, maybe there is a thread that discusses it for Jasmine?
tests/ui/AttachResult.spec.ts
Outdated
it("should be attached when isAttached returns true", (done) => { | ||
isAttachedSpy.and.returnValue(Promise.resolve(true)); | ||
attachResult = Mock.optionsResultComponentSetup(AttachResult, { isAttached: faker.isAttachedMock }, fakeResult); | ||
setTimeout(() => { |
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.
To simulate the back and forth of a server call?
Moved all files inside src for consistency. Added Prettier formatter.
We want this repository to be home to generic and reusable components which could be used by any integrations in the future. So any comment is appreciated!
I started from search-ui-seed and added a basic, generic AttachResult component. The component is super simple and leverages Promises, it's basically a toggle button which also sends analytics events. You just have to give it 3 functions: attach, detach and optionally isAttached. These are configured just like any other component options, there is an example in index.html.