-
Notifications
You must be signed in to change notification settings - Fork 1
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 timestamp for start of turn #1350
Conversation
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 terms of analyzing data: I think it would be better to not set the start_of_turn
variable on each card, but instead, send a start_of_turn
variable within the result
object. This will be saved to the results' json data. The response_interval_ms
can probably be removed (check with @Lia-JX-Li ), as it can be derived from the timestamps.
@@ -62,6 +62,8 @@ const MatchingPairs = ({ | |||
yPosition.current = posY; | |||
} | |||
|
|||
let startOfTurn = performance.now(); |
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.
Does it work like this in React? 🤔 If written like this, I expect startOfTurn
to be overwritten in every re-render... I would expect a state variable that gets set on initialization and to be set again using setStartOfTurn
in the finishTurn
function (same place as you now do the reassignment).
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.
It did work but using a state variable would indeed be better.
@@ -141,7 +142,8 @@ const MatchingPairs = ({ | |||
currentCard.turned = true; | |||
currentCard.noevents = true; | |||
currentCard.boardposition = index + 1; | |||
currentCard.timestamp = performance.now(); | |||
setStartOfTurn(performance.now()); | |||
currentCard.start_of_turn = startOfTurn; |
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.
currentCard.start_of_turn = startOfTurn; |
@@ -152,6 +154,7 @@ const MatchingPairs = ({ | |||
}; | |||
|
|||
const finishTurn = () => { | |||
setStartOfTurn(performance.now()); |
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.
Why set this here and at line 145? Wouldn't it be enough to do it in one of those two places?
Yes, only one is needed. I'm still working on this branch.
5 Nov 2024 15:23:18 Berit ***@***.***>:
…
***@***.**** commented on this pull request.
----------------------------------------
In frontend/src/components/MatchingPairs/MatchingPairs.tsx[#1350 (comment)]:
> @@ -152,6 +154,7 @@ const MatchingPairs = ({
};
const finishTurn = () => {
+ setStartOfTurn(performance.now());
Why set this here and at line 145? Wouldn't it be enough to do it in one of those two places?
—
Reply to this email directly, view it on GitHub[#1350 (review)], or unsubscribe[https://github.com/notifications/unsubscribe-auth/AL34TLHCHGTCPFDZLGU4BYDZ7DIERAVCNFSM6AAAAABRF43OYKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDIMJVG43TQMZSGQ].
You are receiving this because you authored the thread.
[Tracking image][https://github.com/notifications/beacon/AL34TLGZKB7CP37EWKDG46LZ7DIERA5CNFSM6AAAAABRF43OYKWGG33NNVSW45C7OR4XAZNRKB2WY3CSMVYXKZLTORJGK5TJMV32UY3PNVWWK3TUL5UWJTUP7XNBI.gif]
|
start_of_turn is indeed more clear, so I changed it back. I removed the response_interval_ms after consulting with Lia. |
This PR sets a timestamp
start_of_turn
in the result of the first card.First when the board has been loaded, then on every third click of a turn.
Closes: #1298