Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Overlay fix and GA events for error modal #46

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Overlay fix and GA events for error modal #46

wants to merge 6 commits into from

Conversation

nsharma123
Copy link
Collaborator

iisa
iisa approved these changes Jun 8, 2022
View reviewed changes
Copy link
Contributor

@iisa iisa left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

one possible blocker, let's update that piece so we can test the error beacons.

iisa
iisa reviewed Jun 8, 2022
View reviewed changes
src/core/services/loan-token-poller.js
identifier: this.identifier,
isInitial: isInitial,
borrowType: this.borrowType,
interval: this.loanTokenInterval,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blocker: this.loanTokenInterval - seems really heavy to send to GA. can u send this as a boolean or a string like an errorType?

the one we want to catch is if a book has been auto-returned and the interval is polling. is that borrowType?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

setInterval does not return a promise, but an integer number, uniquely identifying the interval timer that was created. so not sure how this heavy to send GA. I just wanted to know if this really have a identification number even after get cleared.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nsharma123 is this PR still good?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iisa iisa iisa approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@nsharma123 @iisa