-
Notifications
You must be signed in to change notification settings - Fork 33
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
Voxel demo #65
Voxel demo #65
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
js-pkg/react/src/main.tsx
Outdated
excludeSelf?: boolean | ||
} | ||
|
||
export function usePresence<T extends Record<string, any>>( |
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.
this change should be in another pr
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 do you think so?
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.
I tend to agree actually, opened #66.
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 do you think so?
So it's easier to revert in isolation, since we squash commits.
"@react-three/drei": "^9.80.2", | ||
"@react-three/fiber": "^8.13.6", | ||
"@types/react-color": "^3.0.6", | ||
"@uiw/react-color": "^1.3.3", |
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.
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.
react-color
hasn't been touched in two years and causes noisy warnings with recent versions of react (github issue)
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.
what do you mean by "off"?
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.
oh, I see what you mean, the colors don't line up by column
js-pkg/react/src/main.tsx
Outdated
const awareness = useAwareness() | ||
const [presence, setPresence] = useState<Map<number, T>>(new Map()) | ||
|
||
const excludeSelf = options?.excludeSelf ?? true |
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.
Minor, but if the default is to exclude self, maybe this option could be called includeSelf
?
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.
good call, I like the principle that bools should be oriented such that the default is false.
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.
A few comments there, otherwise LGTM
Demo: https://y-sweet-demos-git-paulgb-voxel-demo-drifting-corp.vercel.app/voxels