-
Notifications
You must be signed in to change notification settings - Fork 59
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
Fix how hover card is hidden #1105
base: master
Are you sure you want to change the base?
Conversation
@@ -159,12 +159,30 @@ describe('HoverCards', () => { | |||
)) | |||
.request(hover(clock)) | |||
.respondWithData(() => testData.extendedDatasets.last()) | |||
.respondWithProblem(() => entity) |
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 is just a separate typo that I noticed.
unusual, but it could happen if preventHide is set to `true` in | ||
useHoverCard().) But most importantly, if hoverCard.state is `true`, then we | ||
need to hide the current hover card before we show a new one. */ | ||
if (hoverCard.state) hide(); |
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 if
meant that the hide()
function was not called after hoverCard.state
changed to false
.
I also wanted to make a note of how hover cards were being hidden even though the |
.afterResponses(async (component) => { | ||
should.not.exist(document.querySelector('.popover')); | ||
component.should.not.alert(); | ||
// Make sure that this does not result in a Vue warning. | ||
await getAnchor(component).trigger('mouseleave'); | ||
}); | ||
}); | ||
|
||
it('does not log an error if mouseleave is triggered during request', () => { |
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.
In case you're wondering how this test works (there's not much in the way of an assertion), our tests automatically fail if Vue logs a warning. Before the fix, Vue was logging a warning, but with the fix in place, it no longer logs a warning.
@lognaturel noticed that there are cases where mousing away from a link that shows a hover card causes an error to be logged in the browser console. She noticed this pattern:
Looking into it, I think the specific problem was if the user mouses away while the hover card requests are in progress. Doing so is supposed to cancel the requests, but it wasn't doing so. That led to the following sequence:
mouseleave
leads tohoverCard.hide()
being called, which setshoverCard.anchor
tonull
.component.value
is set in theHoverCards
component.computePlacement()
is called. ButcomputePlacement()
needs to know the position ofhoverCard.anchor
. When it tries to figure that out and sees thathoverCard.anchor
isnull
, it throws.computePlacement()
throws triggers thecatch()
in the promise chain, which callsresetResources()
. That clears the response data, but it doesn't resetcomponent.value
orplacement.value
. It shouldn't have to reset those things, because the only thing that's supposed to trigger thecatch()
is a request failure. But the fact thatcomponent.value
is set while the response data is not means that the component attempts to render without any of the data it requires. That leads to an error, and it's that (pretty ugly) error that @lognaturel was seeing in the console.The more general problem here is that
hoverCard.hide()
is not triggering thehide()
function in theHoverCards
component. ThehoverCard
object is just responsible for managing simple state about the hover card, not sending requests. It's thehide()
function that's supposed to cancel requests and resetcomponent.value
andplacement.value
. The fact that it wasn't called was the root cause of the issue.What has been done to verify that this works as intended?
I wrote a test that succeeds now but failed without the code change. I also tried out the fix locally.
Before submitting this PR, please make sure you have:
npm run test
andnpm run lint
and confirmed all checks still pass OR confirm CircleCI build passes