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.

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

chore(log): Add log for image zoom click #1234

Merged
merged 2 commits into from
Jul 14, 2020
Merged

Conversation

mxiao6
Copy link
Contributor

@mxiao6 mxiao6 commented Jul 10, 2020

No description provided.

@mxiao6 mxiao6 requested a review from a team as a code owner July 10, 2020 05:37
Copy link
Contributor

@ConradJChan ConradJChan left a comment

Choose a reason for hiding this comment

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

I'm not sure how useful of a viewer event this would be from an SDK perspective. Is this way preferred over using resin tags to track activity?

src/lib/viewers/image/ImageBaseViewer.js Outdated Show resolved Hide resolved
@mxiao6
Copy link
Contributor Author

mxiao6 commented Jul 10, 2020

I'm not sure how useful of a viewer event this would be from an SDK perspective. Is this way preferred over using resin tags to track activity?

Discussed offline. Summary: I first tried adding resin tag to the imageEl which has mouseup event listener. However resin does’t work in that way. I looked into the resin source code and found the element with resin target tag must be button li etc. Therefore I decided to use event to log.

@@ -373,10 +373,12 @@ class ImageBaseViewer extends BaseViewer {
if (!this.isPannable && this.isZoomable) {
// If the mouse up was not due to panning, and the image is zoomable, then zoom in.
this.zoom('in');
this.emitMetric(VIEWER_EVENT.imageZoomClick, 'in');
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like we already have a zoom event emitted from the viewers -- could we enhance that to include an interactionType or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

zoom event emitted from viewers doesn't get logged. Adding listener to viewer to specifically log zoom event in Preview.js adds a lot of complications to this PR. Instead, I just changed the event name to zoom for scalability and consistence.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In what way does it add complexity?

Copy link
Contributor Author

@mxiao6 mxiao6 Jul 13, 2020

Choose a reason for hiding this comment

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

In what way does it add complexity?

So now it's a two-lines change. If we use emit('zoom'), we need to implement a zoom event handler and attach it in attachViewerListeners in Preview.js, which sounds more complex than two lines.

src/lib/events.js Outdated Show resolved Hide resolved
@mxiao6 mxiao6 requested a review from jstoffan July 14, 2020 18:18
@mergify mergify bot merged commit c29dddc into box:master Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants