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

Add amplitude event for moving rubric window #57931

Merged
merged 2 commits into from Apr 10, 2024

Conversation

Nokondi
Copy link
Contributor

@Nokondi Nokondi commented Apr 10, 2024

Added Amplitude analytics event for reporting when the rubric window is moved. Reports movement start and end positions in two separate events.

Jira Ticket: AITT-554

@Nokondi Nokondi marked this pull request as ready for review April 10, 2024 19:07
Comment on lines +803 to +804
EVENTS.TA_RUBRIC_WINDOW_MOVE_END,
{window_x_end: 0, window_y_end: 0}
Copy link
Member

Choose a reason for hiding this comment

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

this is confusing... shouldn't these be 100/100? are you able to validate that the right values come through in manual testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it works locally. This is something I read about with the way that react testing library renders components. I had a similar issue when writing the unit test for moving the window (i.e., it didn't return the correct values, but it did register that the values had changed). I'll see if I can find a link to what I read before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, meant to say that I've tested manually and gotten the correct results:
image

Copy link
Member

Choose a reason for hiding this comment

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

that would be great. I'm not sure how common this will be, but until it becomes common knowledge I would suggest including a short explanation with a link to the issue as a code comment

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here's the explanation that I pieced together: React Testing Library uses JSDom as the basis for its render function. JSDom doesn't actually render elements, so everything will show up with a position of (0, 0). For the draggable unit test, I was able to get around this because the draggable component applies a transformation to the component, so if you look at the translation, you'll see the change, but the raw x and y position values will always show as (0, 0). It's possible you can get more accurate position readings by using RTL with Jest, but I haven't confirmed that.
image
jsdom/jsdom#1590

Copy link
Member

@davidsbailey davidsbailey left a comment

Choose a reason for hiding this comment

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

lgtm after considering test code comments

@Nokondi Nokondi merged commit 9db36f4 into staging Apr 10, 2024
2 checks passed
@Nokondi Nokondi deleted the add-amplitude-event-for-moving-rubric-window branch April 10, 2024 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants