-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Room UI Redesign: Toolbar Buttons / Menus #3206
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.
lgtm. Left some suggestions but didn't find any major issues
this.updateMyPen(); | ||
}, | ||
|
||
getMyPen() { |
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.
Maybe would expose hasActivePen()
since the only use is:
const hasActivePen = !!scene.systems["pen-tools"].getMyPen();
@@ -5,19 +5,20 @@ import { Popover } from "../popover/Popover"; | |||
import { ToolbarButton } from "../input/ToolbarButton"; | |||
import { ReactComponent as ObjectIcon } from "../icons/Object.svg"; | |||
|
|||
export function PlacePopoverButton({ items, onSelect }) { | |||
export function PlacePopoverButton({ items }) { |
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've mentioned this before but at some point I hope we find a clearer word for this menu than "Place". I suspect "Place" will give people the wrong impression of it being about "this place we're in" (aka the scene), not "place a pen in your hand or an object in the scene". Some alternatives to consider:
- Add
- Create
- Load
- New
- Spawn
- Generate
Similarly, I think "Share" is overloaded in the mobile world and we might want to consider something like, "Broadcast" or "Stream"
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'll bring this up with DPX. Thanks for flagging this 👍
[scene] | ||
); | ||
|
||
// TODO: We probably shouldn't use freeze mode for users spawning emojis on a 2d device. |
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.
+1 - Good idea to either implement a 2D emoji menu or separate the logic that ties their visibility to the frozen state. https://github.com/mozilla/hubs/blob/7159d67ccc84d6dfccf7bf3b1590275f04092367/src/components/emoji-hud.js#L88-L91
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.
I especially like that when Discord's emoji button is hovered, it will change the emoji each time. I would like to do the same for our emoji button. Maybe it would fit for our "Place" menu too.
import { ReactComponent as ReactionIcon } from "../icons/Reaction.svg"; | ||
import { ToolbarButton } from "../input/ToolbarButton"; | ||
|
||
export function ReactionButtonContainer({ scene }) { |
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.
We'll need to do some thinking around the UX for "send to 3D world" vs "send to chat" vs "send to both" . It is surprising to me that there's a "react" (emoji) menu in the toolbar rather than in the chat sidebar or the "Place" popover menu
I want to cover the reactions/emojis issues in detail in another PR. But I agree with all of these concerns. I've created two issues to cover the Reactions menu and emojis in chat. I think these are two separate issues and deserve two separate spots in the UI. However, we need to discuss how each of them will work. |
This PR includes the functionality for the share popover, place popover, reaction/emoji menu, and leave button.
I'm not a huge fan of the way the reaction/emoji menu turned out. The original plan for phase one of the redesign was to use the existing menu but this puts you into freeze mode. I think we should go with this concept instead:
When you click one of the emojis, it'll spawn in the world and you can still grab it.