-
Notifications
You must be signed in to change notification settings - Fork 50
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
Collab: Add presence avatars #44
Conversation
Code and styles look good! Not sure if there's anything to fix here, but from the Storybook page, I noticed a few things:
Other than that, 👍 🚢 |
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.
Great work Lena
Thanks for testing!
Good one, I've been wondering about this too. @johngodley Do you think we should backport some of the responsive styles we have in p2-editor for the stacked toolbar?
Yeah, this Storybook maneuver (and also how Storybook handles hot reloading) doesn't properly unmount the React component, so we accumulate "ghosts" like this. Fortunately it's just a Storybook dev thing. |
It's part of a bigger, more complicated, issue that I've been carefully trying to avoid! The core editor now only uses a two row toolbar, there is no single row option: This is fine for the core editor, but can look very bulky in an isolated editor (comments, for example). The rows are also in very different places in the DOM and don't merge well with CSS modification. There's an open issue here (#19) to improve the toolbar in general, and the two row issue is now a blocker to remove this (#25) deprecated warning. In the short term I think whatever is the easiest way to get things 'acceptable' is ok. For example, maybe the avatars can squish together a bit more on a smaller screen. I also note that in Anne's screenshot the block mover arrows are incorrectly displayed, using up additional width. In the longer term we need a better plan for handling toolbars, maybe just accepting the two row situation and adjusting around it, or maybe forcing an inline popup toolbar for smaller widths and situations that need less UI. |
This is not really a full fix for the responsive styling problem (#44 (comment)), but just to make it consistent with the other toolbar elements.
32034a4
to
546630f
Compare
That makes sense. Thanks for the explanation! |
A different signal for smaller ports (like a dot or icon) that opens a panel on hover to show the avatars could work. This can also be true to see all the avatars, hovering the +, instead of the tooltip? |
@evilluendas maybe you all discussed colors, but would they align with the P2 brand colors, the specific-P2 configuration, or the Workspace's? |
In terms of priority, a narrow-viewport-specific design is not particularly necessary right now because in a properly styled use case (like in P2), the toolbars will be stacked and have enough space. I was wondering about colors, too. We did not discuss, but I picked out several legible colors that were somewhat distinct and matched the muted palette of the wp-admin colors.
For downstream P2 usage, we can override these as we want, perhaps less muted. |
We don't have an extended color palette defined yet, but this could be a good opportunity to generate it. I'll open an issue with specific colors so we can try them 👍 |
Based on #43
Changes proposed
Adds presence avatars to the editor toolbar to show peers who are present.
It displays up to three images, and the rest is overflow. (We can revisit this number once we gather some usage stats.)
Testing instructions
yarn install && yarn storybook