-
Notifications
You must be signed in to change notification settings - Fork 178
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
feat(app): Add attached pipette info to intercom support #2140
Conversation
Codecov Report
@@ Coverage Diff @@
## edge #2140 +/- ##
==========================================
- Coverage 31.18% 31.15% -0.03%
==========================================
Files 467 467
Lines 7689 7696 +7
==========================================
Hits 2398 2398
- Misses 5291 5298 +7
Continue to review full report at Codecov.
|
f9a7942
to
cde177f
Compare
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.
Maybe we should avoid intercom = noop
altogether and always do window.Intercom && window.Intercom(args)
? It's more verbose but IMO much easier to understand. This slippery bug came from mutating intercom
in a weird way. With this change, if somehow the intercom script doesn't inject Intercom
into window scope, then Run App will crash, which I think is what Mike was trying to avoid?
Or instead of window.Intercom && window.Intercom(stuff)
each time, we could do
function callIntercom (...args) {
if (window && window.Intercom) {
window.Intercom(...args)
} else {
log.warn('window.Intercom is not available')
}
}
then call that fn instead of intercom
/ window.Intercom
Will rebase this PR once #2179 is merged |
cde177f
to
1a8078b
Compare
Testing with @umbhau
|
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.
🌭
Requested changes implemented in #2179
From Windows Computer: Connect to a robot
Works, just have to refresh intercom a few times to get the fields to update. Doubt we can do anything to fix 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.
💃
overview
This PR closes #2019 by adding attached pipettes info to intercom data when a the api returns
'api:PIPETTES_SUCCESS'
changelog
review requests
Unfortunately, the imported intercom handler always returned as a
noop
forIntercom('update')
calls. So we had to do it the old fashioned way withwindow.Intercom(...)
Intercom does not accept arrays or objects for values in custom data fields. So rather than just have a single data attribute
Pipettes: {...}
with all of the apps info about the pipettes, it needed to be split up intoLeft Pipette: model string
etc.TODO: (in the future)
Add pipette serial numbers to intercom once they are exposed by the api
We have a test intercom app for these PRs so we can avoid confusing support with our nonsense messages. I will ping you with the intercom ID you need to set in app, and a link to the dashboard to view the user data
Open app (with the intercom ID set that I share via slack)
Also go to your Intercom user in the intercom webapp. Look for the "Details" section on the left of the page. To see our custom properties, you will have to click "Show __ Hidden" link.