fix: tweak the anonymous footer component design #20884
Conversation
melvin-bot
bot
requested review from
aimane-chnaif
and removed request for
a team
|
@aimane-chnaif Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| return ( | ||
| <View style={styles.anonymousRoomFooter}> | ||
| <View | ||
| onLayout={onLayout} |
There was a problem hiding this comment.
Why changed withWindowDimensions to onLayout? This callback seems slower.
There was a problem hiding this comment.
This is needed because we need to change the layout of this component based on the component width rather than the screen size.
There was a problem hiding this comment.
I think we can calculate width enough from windowWidth. LHN width is fixed as 375.
There was a problem hiding this comment.
We can, but the component won't be fully responsive. Like it will fail inside storybook where there is no left side bar.
There was a problem hiding this comment.
My concern is when resize window, layout flickers as you see my video.
I think storybook is less important than that.
There was a problem hiding this comment.
@marcochavezf Can you share your thoughts here?
There was a problem hiding this comment.
Bump @marcochavezf!
There was a problem hiding this comment.
Oh this is an interesting issue, yeah my initial thought was that the flickering was almost unnoticeable in the video and I agree that an end-user resizing the window would be a rare case. But after seeing the video that @aimane-chnaif posted here, I think the problem is more noticeable when the room is loading on a small screen. It seems that we're not loading the sign-in button for a moment, not sure if it's related to the resizing logic, but agree with @aimane-chnaif we should fix that (at least the sign-in button which causes the report footer to be displayed incorrectly for a moment).
There was a problem hiding this comment.
This is clearly noticeable when app becomes heavy (i.e. HT account, old device). View onLayout is called a few seconds after mount.
There was a problem hiding this comment.
@aimane-chnaif Removed onLayout. Please check.
| <View style={[props.isSmallScreenWidth ? styles.alignItemsStart : styles.alignItemsEnd]}> | ||
| <View style={styles.anonymousRoomFooterWordmarkAndLogoContainer(isSmallSizeLayout)}> | ||
| <View style={[styles.mr4, styles.flexShrink1]}> | ||
| <View style={[isSmallSizeLayout ? styles.alignItemsStart : styles.alignItemsEnd]}> | ||
| <ExpensifyWordmark style={styles.anonymousRoomFooterLogo} /> | ||
| </View> | ||
| <Text style={[styles.textNormal, styles.textWhite]}>{props.translate('anonymousReportFooter.logoTagline')}</Text> |
There was a problem hiding this comment.
No font family is added here. #20494 (comment)
@aimane-chnaif This was meant to be handled here. No? |
you mean loooong room name case is out of scope for this PR? |
That is my understanding. Correct me if I'm wrong @marcochavezf. |
|
I think the main purpose of this PR is to fix wrong designs different from design doc.
|
|
Yes, @aimane-chnaif is right. Only the long name is out of scope here since it would be fixed in the other issue. We're aiming to fix all the style inconsistencies here 🙇🏽♂️ |
|
"Join in on the discussion" still doesn't feel like it's the right font - can you double check that? An easy way to tell is to use a lowercase "g" and see if it is double story or not. Then for the background color, it looks incorrect as well - it should be the same background color as a preview card or the LHN or an IOU for example. |
@shawnborton I'm using I've updated the background colour FYI. |
|
That's the right font, but it doesn't look like it's working. Can you add a lowercase g to the sentence so I can check? |
|
@shawnborton Here's how it looks. 
|
|
Okay great, thanks for confirming! That looks good to me. |
|
Still waiting for @marcochavezf's thoughts on this thread. This is my concern: Screen.Recording.2023-06-23.at.1.59.57.PM.mov |
|
@allroundexperts please pull main |
| const hideComposer = isArchivedRoom || !_.isEmpty(props.errors) || !isAllowedToComment; | ||
|
|
||
| const isSmallSizeLayout = props.windowWidth - (props.isSmallScreenWidth ? 0 : 375) < variables.anonymousReportFooterBreakpoint; |
There was a problem hiding this comment.
| const isSmallSizeLayout = props.windowWidth - (props.isSmallScreenWidth ? 0 : 375) < variables.anonymousReportFooterBreakpoint; | |
| const isSmallSizeLayout = props.windowWidth - (props.isSmallScreenWidth ? 0 : variables.sideBarWidth) < variables.anonymousReportFooterBreakpoint; |
src/styles/styles.js
Outdated
| flexDirection: 'row', | ||
| alignItems: 'center', | ||
| ...(isSmallSizeLayout && { | ||
| // flex: 1, |
There was a problem hiding this comment.
| // flex: 1, |
| <View style={styles.mr4}> | ||
| <View style={[props.isSmallScreenWidth ? styles.alignItemsStart : styles.alignItemsEnd]}> | ||
| <View style={styles.anonymousRoomFooterWordmarkAndLogoContainer(props.isSmallSizeLayout)}> | ||
| <View style={[styles.mr4, styles.flexShrink1]}> |
There was a problem hiding this comment.
| <View style={[styles.mr4, styles.flexShrink1]}> | |
| <View style={[props.isSmallSizeLayout ? styles.mr1 : styles.mr4, styles.flexShrink1]}> |
There was a problem hiding this comment.
I request this change because 4px is enough for space.
Before fix: (no need to wrap)
After fix:
Reviewer Checklist
Screenshots/VideosWebweb.movMobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android
|
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
|
Sorry I found issue while testing edge cases. It doesn't make sense to show anonymous footer in archived room. (already hiding on main though was not intentional) Also, I found bug on main that offline indicator not showing when offline. We can fix that as well here. So we can update conditions like this: const isAnonymousUser = Session.isAnonymousUser();
- const hideComposer = isArchivedRoom || !_.isEmpty(props.errors) || !isAllowedToComment;
+ const hideComposer = isArchivedRoom || !_.isEmpty(props.errors) || !isAllowedToComment || isAnonymousUser;
const isSmallSizeLayout = props.windowWidth - (props.isSmallScreenWidth ? 0 : variables.sideBarWidth) < variables.anonymousReportFooterBreakpoint;
return (
<>
- {(isArchivedRoom || hideComposer || isAnonymousUser) && (
+ {hideComposer && (
<View style={[styles.chatFooter, props.isSmallScreenWidth ? styles.mb5 : null]}>
- {isAnonymousUser && (
+ {isAnonymousUser && !isArchivedRoom && (
<AnonymousReportFooter
report={props.report}
isSmallSizeLayout={isSmallSizeLayout}
/>
)}
{isArchivedRoom && <ArchivedReportFooter report={props.report} />}
{!props.isSmallScreenWidth && (
<View style={styles.offlineIndicatorRow}>{hideComposer && <OfflineIndicator containerStyles={[styles.chatItemComposeSecondaryRow]} />}</View>
)}
</View>
)}
- {!hideComposer && (props.shouldShowComposeInput || !props.isSmallScreenWidth) && !isAnonymousUser && (
+ {!hideComposer && (props.shouldShowComposeInput || !props.isSmallScreenWidth) && (
<View style={[chatFooterStyles, props.isComposerFullSize && styles.chatFooterFullCompose]}> |
Cool, Fixed @aimane-chnaif! |
|
@allroundexperts please test again all possible cases on latest codebase: normal/archived rooms, web/resized web/native, online/offline |
I've not tested this on native since deep links seem not be working again. Can you confirm this on your end? |
|
You can test by updating Session.isAnonymousUser() condition manually. This condition should not affect styles made in this PR. |
|
|
Could we make the Sign In button just slightly less wide? Maybe take like 12-16px off of it. This way we have better space available for small viewports. |
|
Otherwise this feels good to me! |
@allroundexperts ^
SNH is over? |
|
@shawnborton Here is how it looks after removing 15px. 
|
|
Bump @shawnborton! |
|
Cool, that works for me, thanks! |
|
✋ 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 https://github.com/marcochavezf in version: 1.3.38-0 🚀
|
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.38-7 🚀
|
Details
This PR changes the design of the
AnonymouseFootercomponent such that it matches the specs defined here.Fixed Issues
$ #20699
PROPOSAL: #20699 (comment)
Tests
Offline tests
N/A
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.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)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Web
Mobile Web - Chrome
Mobile Web - Safari
Desktop
iOS
Android