-
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
feat: Add pronoun field to entry modal for users #6116
Conversation
<a-entity class="nametag-text" | ||
text="side: double; textAlign: center; color: #ddd; fontSize: 0.1;" text-raycast-hack | ||
position="0 0.025 0.001"></a-entity> | ||
<a-entity class="pronouns-text" |
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 is the only new line here.
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 we also want to update the text color based on the theme for the pronoun text elements we should add it here:
https://github.com/mozilla/hubs/pull/6116/files#diff-f010909f986ed471df83ed99850af7a72b52d0433d806c250731df2f5b767629L282
Looks mostly good, I just pointed out a few things. Thanks for adding this!
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 is almost ready, just a couple of nits.
src/components/name-tag.js
Outdated
this.updateHandRaised(); | ||
this.resizeNameTag(); | ||
} | ||
}, | ||
|
||
updateNametagWidth(name) { | ||
if (name) { | ||
this.pronounsText?.el && this.pronounsText.el.components["text"].getSize(this.size); |
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 think both pronounText
and nametagText
should exist at this point so we can probably remove all the defensive code from here. We can probably also remove the name
param and just pass the method name to the listener
this.nametagText.el.addEventListener("text-updated", this.updateNametagWidth, { once: true})
as long as we bind it here:
https://github.com/mozilla/hubs/blob/79f2fc11836c4573c546fb980f12c83afd1fb0c4/src/components/name-tag.js#L64
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.
The check for nametagText.el
was legacy code, but I removed and you're correct, works fine! 👍
From this UX Blast document: https://docs.google.com/document/d/13K_N5eBXk0aTBlHVoPUPIxpSWFeY2rP2EafVqS30fg4/edit?skip_itp2_check=true#heading=h.xcm1taufnky6