-
Notifications
You must be signed in to change notification settings - Fork 19
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
Workspace improvements #178
Conversation
Your Render PR Server URL is https://ozone-staging-pr-178.onrender.com. Follow its progress at https://dashboard.render.com/web/srv-crcg9krqf0us73fcvkk0. |
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.
Looks good.
useInfiniteQuery({ | ||
queryKey: ['followers', { id }], | ||
queryFn: async ({ pageParam }) => { | ||
const { data } = await labelerAgent.api.app.bsky.graph.getFollowers({ |
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.
nit: .api.
no longer required (deprecated)
const { data } = await labelerAgent.api.app.bsky.graph.getFollowers({ | |
const { data } = await labelerAgent.app.bsky.graph.getFollowers({ |
components/workspace/hooks.tsx
Outdated
// Only when pass the onClose here because if we pass it during instantiation, the update essentially triggers onClose | ||
// causing the toast to close indefinitely |
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.
Doesn't updating twice also cause this behavior ?
Would keeping another reference help ?
const toastNr = useRef<number>(0)
const toastId = useRef<number | string | null>(null)
// ...
onSuccess: () => {
// ...
const currentNr = ++toastNr.current
const onClose = () => {
// Only clear the toastId ref when the latest toast is closed
if (toastNr.current === currentNr) toastId.current = null
}
// use onClose both for init and update
// ...
},
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.
thanks ended up using the isActive check.
) | ||
.forEach((checkbox) => { | ||
if (results.failed.includes(checkbox.value)) { | ||
checkbox.checked = 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.
If I'm not mistaken, this could cause the react component's internal state not to be in sync with the actual checkbox state, leading to unexpected behavior. Indeed, setting checkbox.checked
won't trigger the change listener that the react component typically listens for in order to keep the internal state in sync.
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 catch! the internal state is only kept for a UX detail which let's us do gmail style shift+click checkbox selection. which isn't that important to the functionalities. thanks for pointing this out, we're now triggering the mousedown event manually along with checked
value update
This PR adds a bunch of new improvements to workspace feature
4 Add all follows of a user to workspace.