Add Workspace name label next to room name in the chats list#11799
Conversation
melvin-bot
Bot
requested review from
mananjadhav and
marcochavezf
and removed request for
a team
|
CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
This is looking really good! Just a few design details:
|
|
I have updated the code and the screenshots |
|
For this screenshot here: I don't think we want the |
There was a problem hiding this comment.
Requested some smaller changes. @bernhardoj I would recommend add tests and screenshots for workspace name with ellipsis.
Also can we please add elaborate QA steps, this would involve, maybe identifying the #announce and #admin named chat?
There was a problem hiding this comment.
Instead of doing this, we can stick to the existing colors.gray values and pass the style as opacity: 0.5
There was a problem hiding this comment.
If we apply the opacity style to the text, it will affect the background color and the text itself. I was thinking that we could take the icon color and append the string with '80' (the hex will be #C6C9CA80), which is the hex code for the 50% opacity. So, it will look like this
StyleUtils.getBackgroundColorStyle(themeColors.icon + '80')
but I think it looks ugly, that's why I decided to add a new color.
If we still want to apply the opacity style, I think we need to change the TextPill component a bit by wrapping the text with View.
What do you think?
There was a problem hiding this comment.
I am fine with wrapping the View around Text.
There was a problem hiding this comment.
Okay, so it turns out that wrapping it with View and apply the opacity to the View also affects the Text. So, I think the only way is to apply the opacity to the backgroundColor.
There was a problem hiding this comment.
Should I keep the iconLight color?
There was a problem hiding this comment.
As far as I know, RN does not have such thing, but we can make our own implementation. I am thinking that maybe we could make a new function in StyleUtils.js let say getBackgroundColorWithOpacityStyle which takes the hex code as string and the opacity as number (0 to 1). Then, we will convert that hex to rgb numbers then return rgba(r, g, b, opacity). It will look like this:
function getBackgroundColorWithOpacityStyle(background, opacity) {
const r = ....
const g = ....
const b = .....
return {
backgroundColor: `rgba(${r}, ${g}, ${b}, ${opacity})`
}
}
What do you think?
There was a problem hiding this comment.
I will lean on @mananjadhav and @marcochavezf to evaluate that, but I like the idea.
There was a problem hiding this comment.
@bernhardoj Can we not just use rgb(r, g, b, 0.5) in the colors.js ?
There was a problem hiding this comment.
Yes, we can remove the iconLight and use the new function above that I propose like this:
StyleUtils.getBackgroundColorWithOpacityStyle(themeColors.icon, 0.5).
There was a problem hiding this comment.
My newest commit currently uses above approach. Let me know if there is any concern.
There was a problem hiding this comment.
Can we rename this to something related to text say title? text. etc.?
children is generally used for nodes or React components and looks misleading here.
There was a problem hiding this comment.
Instead of setting this empty, we should make the property mandatory
Fixed. I also put some newest screenshots with long workspace name and updated the test steps. |
There was a problem hiding this comment.
@marcochavezf @shawnborton Can one of you confirm if this is the correct accessibilityLabel ?
There was a problem hiding this comment.
Deferring to @marcochavezf as I am unfamiliar with this one.
There was a problem hiding this comment.
Ah good question, since this only be used for Workspace name atm let's change the value to Workspace name
There was a problem hiding this comment.
Not sure I understand this change. Are we removing this style? This is used at two places (OptionRowLHN, and also OptionRow). I think removing this will break the styling in OptionRow.
There was a problem hiding this comment.
Sorry, I missed that it is also being used at OptionRow component. I merge both styles to fix the #announce text got truncated on not compact mode. I will fix it later. Thanks for pointing it out!
|
@bernhardoj Some more comments. |
bernhardoj
force-pushed
the
feat/#11543
branch
from
cf21ec4 to
1731e74
Compare
|
Screenshots look pretty good to me! @bernhardoj could you also try pinning one of these rooms that has a super long name, in both compact and normal mode, and sharing screenshots please? Thanks! |
|
@shawnborton I have updated the screenshots. |
|
Awesome, screenshots look good to me! Thanks @bernhardoj |
mananjadhav
left a comment
mananjadhav
left a comment
There was a problem hiding this comment.
Thanks for the changes and being patient with this @bernhardoj.
🎀 👀 🎀
C+ reviewed
|
@marcochavezf @shawnborton Here's the checklist.
ScreenshotsWeb
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
|
marcochavezf
left a comment
marcochavezf
left a comment
There was a problem hiding this comment.
Thank you guys looks mostly good to me! Just a comment related to displayNameStyle
| const displayNameStyle = StyleUtils.combineStyles(props.viewMode === CONST.OPTION_MODE.COMPACT | ||
| ? [styles.optionDisplayName, ...textUnreadStyle, styles.optionDisplayNameCompact, styles.mr2] | ||
| : [styles.optionDisplayName, ...textUnreadStyle], props.style); | ||
| const displayNameStyle = StyleUtils.combineStyles([styles.optionDisplayName, styles.optionDisplayNameCompact, ...textUnreadStyle], props.style); |
There was a problem hiding this comment.
Ah why are we merging styles.optionDisplayName and styles.optionDisplayNameCompact here? Originally we were only appending styles.optionDisplayNameCompact if props.viewMode === CONST.OPTION_MODE.COMPACT
There was a problem hiding this comment.
When the viewMode is Compact, we put the latest chat text next to DisplayName and we append the style.optionDisplayNameCompact to make sure DisplayName will take the whole available width. Now, we have TextPill next to DisplayName, that's why I append it no matter what the viewMode is. If we don't do this, the DisplayName will get truncated like @shawnborton previously mentioned here.
For this screenshot here:
I don't think we want the
#announceroom to truncate unless it was truly wider than the total available horizontal width of that row. Let me know if that makes sense.
There was a problem hiding this comment.
Any update on this?
There was a problem hiding this comment.
Oh thanks for the explanation @bernhardoj, sorry for the delay here
bernhardoj
dismissed stale reviews from mananjadhav and shawnborton
via
57b6006
bernhardoj
force-pushed
the
feat/#11543
branch
from
57b6006 to
ca9bc6b
Compare
marcochavezf
left a comment
marcochavezf
left a comment
There was a problem hiding this comment.
Thank you guys, LGTM
|
The PR checklists were already filled out, but the GH actions are failing because the text of one checkbox changed. So I'm going to merge it to unblock this change. |
|
@marcochavezf looks like this was merged without the checklist test passing. Please add a note explaining why this was done and remove the |
|
|
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
|
🚀 Deployed to staging by @marcochavezf in version: 1.2.20-0 🚀
|
|
🚀 Deployed to production by @chiragsalian in version: 1.2.20-3 🚀
|
5 participants
Details
Add Workspace name label next to room name in the chats list
Fixed Issues
$ #11543
PROPOSAL: #11543 (comment)
Tests
To test, you need to be at least in a workspace chat room.
a. is not truncated
b. has a workspace name shown next to the chat room name
PR Review Checklist
PR Author Checklist
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)PR Reviewer Checklist
The reviewer will copy/paste it into a new comment and complete it after the author checklist is completed
### Fixed Issuessection aboveTestssectionQA stepssectiontoggleReportand notonIconClick).src/languages/*filesSTYLE.md) were followedAvatar, I verified the components usingAvatarhave been tested & I retested again)/** comment above it */thisproperly so there are no scoping issues (i.e. foronClick={this.submit}the methodthis.submitshould be bound tothisin the constructor)thisare necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);ifthis.submitis never passed to a component event handler likeonClick)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)Avataris modified, I verified thatAvataris working as expected in all cases)QA Steps
To test, you need to be at least in a workspace chat room.
a. is not truncated
b. has a workspace name shown next to the chat room name
Screenshots
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android