-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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 #23130] Add custom status settings to profile #23267
[Feat #23130] Add custom status settings to profile #23267
Conversation
eccc84e
to
ccd3017
Compare
…into feat/#Expensify#23130-status-page-settings
…into feat/#Expensify#23130-status-page-settings
…into feat/#Expensify#23130-status-page-settings
…into feat/#Expensify#23130-status-page-settings
children: PropTypes.node.isRequired, | ||
|
||
/** The illustration to display in the header. Can be either an SVG component or a JSON object representing a Lottie animation. */ | ||
// illustration: PropTypes.oneOfType([PropTypes.func, PropTypes.object]).isRequired, |
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.
do we need 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.
oops, cleaned
thanks!
<HeaderWithBackButton | ||
// eslint-disable-next-line react/jsx-props-no-spreading | ||
{...propsToPassToHeader} | ||
titleColor={backgroundColor === themeColors.appBG ? undefined : themeColors.textColorfulBackground} |
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 we can define a variable and put this backgroundColor === themeColors.appBG ? undefined : themeColors.textColorfulBackground - in case it's duplicating multiple times
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 first one ending with textColorfulBackground
and the second with iconColorfulBackground
so they are different
I moved it out of the render
style={styles.staticHeaderImage} | ||
/> | ||
</View> | ||
<View style={[styles.pt5]}>{children}</View> |
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.
no needs for []
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.
👍🏻
User.clearDraftCustomStatus(); | ||
}; | ||
|
||
const footerComponent = useMemo( |
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 will say that here we do not need memoization - because you returning JSX which is not a heavy calculation - also moving this code directly in render - removes not needed useCallback for updateStatus - because Button component is still a Class which is extended on Component (not PureComponent) and it will be re-rendered in any case.
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.
As we gradually migrate all Class components to FC, it's prudent to prepare our code accordingly. Even if the Button component were to be a highly optimized FC, the onPress={updateStatus}
would still trigger a re-render. So, I think it makes sense to use useCallback
here, as well as useMemo
. @hannojg, what do you think?
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.
Not sure if i agree with re-render for button - if it will be correctly specified with React.memo - it will be re-renders only if any of the props will be changed. Also if we talking about FC - i see that a lot of components still not have memo wrappers - which means that most of the useCallbacks still not needed, as an example in your code a little above src/components/EmojiPicker/EmojiPickerButtonDropdown.js - you have onPress function which is not wrapped in useCallback, because PressableWithoutFeedback - is not memoized. Also your useCallback will be re-creates every time because defaultEmoji, and defaultText are not memoized and will be new for any re-render. And making them memoized doesn't make any sense because both are not expensive calculations - as the result - memoized all that items will not bring and performance improvements but will keep more memory :-)
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.
👍🏻 okay, I'll remove it
function StaticHeaderPageLayout({backgroundColor, children, image: Image, footer, imageContainerStyle, style, ...propsToPassToHeader}) { | ||
const {windowHeight} = useWindowDimensions(); | ||
|
||
const titleColor = useMemo(() => (backgroundColor === themeColors.appBG ? undefined : themeColors.textColorfulBackground), [backgroundColor]); |
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.
const {titleColor, iconFill } = useMemo(() => {your logic here}) - you have the same check backgroundColor === themeColors.appBG - and both logic can be combined in one.
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.
👍🏻
Reviewer Checklist
Screenshots/VideosWebweb.mov8mb.video-0YB-PWNy2X2C.mp4Mobile Web - Chromeandroid-web.movMobile Web - Safariios-web.movDesktopdesktop.moviOSios.movAndroidandroid.mov |
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.
Some code clean up should be made - but those changes will not really effect user experience - i tested on all platforms - working fine. We need to get approves from design team and internal dev as well
🎀 👀 🎀 C+ reviewed
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.
Code is looking pretty good! Just a couple of small comments
@@ -73,6 +75,15 @@ function ProfilePage(props) { | |||
pageRoute: ROUTES.SETTINGS_CONTACT_METHODS, | |||
brickRoadIndicator: contactMethodBrickRoadIndicator, | |||
}, | |||
...(Permissions.canUseCustomStatus(props.beta) |
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.
...(Permissions.canUseCustomStatus(props.beta) | |
...(Permissions.canUseCustomStatus(props.betas) |
This variable is wrong
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.
👍🏻
inputID="test" | ||
onPress={() => Navigation.navigate(ROUTES.SETTINGS_STATUS_SET)} | ||
/> | ||
<MenuItemWithTopDescription |
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.
Let's get rid of this for now since it doesn't do anything. We can add it in the PR for making this work
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.
👍🏻
const hasDraftStatus = !!draftEmojiCode || !!draftText; | ||
|
||
const updateStatus = useCallback(() => { | ||
const endOfDay = new Date(); |
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 that we can do this cleaner/easier using moment.js endOf
method
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.
👍🏻
const defaultEmoji = lodashGet(draftStatus, 'emojiCode') || lodashGet(currentUserPersonalDetails, 'status.emojiCode', '💬'); | ||
const defaultText = lodashGet(draftStatus, 'text') || lodashGet(currentUserPersonalDetails, 'status.text', ''); | ||
|
||
const onSubmit = (v) => { |
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.
Why v
? I think we should change this to have a more descriptive variable name
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.
Just a shortcut. I replaced it with the fully spelled version
…pensify#23130-status-page-settings
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.
Looking great just one last comment!
✋ 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/stitesExpensify in version: 1.3.53-0 🚀
|
🚀 Deployed to production by https://github.com/yuwenmemon in version: 1.3.53-2 🚀
|
const emojiPopoverAnchor = useRef(null); | ||
useEffect(() => EmojiPickerAction.resetEmojiPopoverAnchor, []); | ||
|
||
const onPress = () => |
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.
Coming from #29093
I wouldn't call it a regression per se, but rather a case that was missed/not planned, but the button didn't toggle an emoji picker (i.e. pressing a button when the emoji picker is already open didn't close it)
onPress={onPress} | ||
nativeID="emojiDropdownButton" | ||
accessibilityLabel="statusEmoji" | ||
accessibilityRole="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.
Setting accessibilityRole
as text
caused #29097. This should have been button
instead.
Details
This is the first part of a series of PRs to implement the UI for the new custom status feature.
It implements the Status option in the profile settings, and the screen to set a status.
As this feature is incomplete yet and build in iterations it will be hidden behind the
customStatus
beta.Fixed Issues
$ #23130
PROPOSAL:
Tests
customStatus
beta to your account or simply let this function return trueOffline tests
QA Steps
PR Author Checklist
### Fixed Issues
section aboveTests
sectionOffline steps
sectionQA steps
sectiontoggleReport
and notonIconClick
)myBool && <MyComponent />
.src/languages/*
files and using the translation methodWaiting for Copy
label for a copy review on the original GH to get the correct copy.STYLE.md
) were followedAvatar
, I verified the components usingAvatar
are working as expected)/** comment above it */
this
properly so there are no scoping issues (i.e. foronClick={this.submit}
the methodthis.submit
should be bound tothis
in the constructor)this
are necessary to be bound (i.e. avoidthis.submit = this.submit.bind(this);
ifthis.submit
is never passed to a component event handler likeonClick
)StyleUtils.getBackgroundAndBorderStyle(themeColors.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)ScrollView
component to make it scrollable when more elements are added to the page.main
branch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTest
steps.Screenshots/Videos
Web
Screen.Recording.2023-08-06.at.14.56.40.mov
Mobile Web - Chrome
Screen.Recording.2023-08-06.at.15.03.19.mov
Mobile Web - Safari
mWebIos.mp4
Desktop
Screen.Recording.2023-08-06.at.15.02.11.mov
iOS
Simulator.Screen.Recording.-.iPhone.14.Pro.-.2023-08-06.at.15.08.02.mp4
Android
telegram-cloud-document-2-5366357118599312182.mp4