-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Account quick switch on web #7190
base: main
Are you sure you want to change the base?
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.
Yesssss, I love it. I think we should add a quick hover state and probably some icons, what do you think?
Usability question: Lets say you are viewing a specific post, eg. https://bsky.app/profile/echarles.net/post/3lehhnon3ns2t and you switch account via the profile picture menu, will you stay on the same post, or will you be driven to the new account home page? I have not looked at the code of this PR ... as a user, I would rather stay on the post I was looking at. Pretty sure others will have other opinions :) |
7d15c4a
to
72ccecd
Compare
No, out of scope for this PR. would be nice though :) |
6ea752f
to
62c3b9f
Compare
@@ -119,6 +119,11 @@ export function Trigger({children, label, role = 'button'}: TriggerProps) { | |||
} = useInteractionState() | |||
const {state: focused, onIn: onFocus, onOut: onBlur} = useInteractionState() | |||
|
|||
const prevControlIsOpen = React.useRef(control.isOpen) |
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.
Use state for this instead, it's not legit to modify refs during render.
https://react.dev/reference/react/useState#storing-information-from-previous-renders
We have a bunch of code like this, look for prev
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 LGTM (and I love it) but wanna hear back from Dan on if we can handle prev state more correctly
This reverts commit 050ab95.
a13bbe4
to
5391648
Compare
Clicking the profile picture in the top left now shows an account quick switch menu
Animation is disabled for prefers-reduced-motion
Screen.Recording.2025-01-06.at.19.39.46.mov
Test plan