-
Notifications
You must be signed in to change notification settings - Fork 3
feat: indicate tracks from other users #975
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
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.
@ErikSin I noticed this is failing CI. Can you fix that before I review, please?
const {data: deviceInfo} = useDeviceInfo(); | ||
|
||
const isMine = | ||
createdByDeviceId && createdByDeviceId === deviceInfo?.deviceId; |
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.
would it make any sense to put this in a hook or function and returns a simple boolean?
useIsMine(originalVersionId) or something?
Then the render function in the list item is more about layout and style?
status: originalVersionIdToDeviceIdQueryStatus, | ||
} = useOriginalVersionIdToDeviceId(observation.originalVersionId); | ||
const isMine = createdByDeviceId === deviceInfo?.deviceId; | ||
const queriesSucceeded = |
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.
Is it worth still checking if deviceInfo or createdByDeviceId is still loading? If so, would the isMine be incorrect (in the new file) and thus apply the incorrect styling? (would default to not isMine when I am not sure that is the more likely scenario)
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.
Some suggestions and questions but otherwise seems fine.
closes #936