-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Allow Welcome Message on Room Creation #29541
Conversation
@eVoloshchak 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] |
Deploying with
|
Latest commit: |
c56634a
|
Status: | ✅ Deploy successful! |
Preview URL: | https://82f54dd7.helpdot.pages.dev |
Branch Preview URL: | https://vit-27836followup.helpdot.pages.dev |
Reviewer Checklist
Screenshots/VideosWebScreen.Recording.2023-10-13.at.14.52.00.movMobile Web - Chromescreen-20231013-150756.mp4Mobile Web - SafariRPReplay_Final1697201811.MP4DesktopScreen.Recording.2023-10-13.at.15.12.41.moviOSRPReplay_Final1697201677.MP4Androidscreen-20231013-150549.mp4 |
@mountiny, there's another issue on iOS (which was also present in the previous PR, but nobody except me could reproduce it so we weren't sure if it was a problem with my specific setup), but I can see it's also present in your video RPReplay_Final1697195440.MP4Should we fix this one here too? |
@mountiny This is missing the actual fix 😅 The behavior is still set to height. It should be padding. The attached video have the keyboard flashing once, it should not flash at all. |
Confirmed the problem doesn't exist after setting ![]() |
Pushed! |
Thanks for noticing, I think that this is not blocker, I am not sure how to fix this right now and cant debug it so unless you know from top of you head how to fix this, I think we should move ahead. It might be the combination of Form, KeyboardAvoidingView and TabNavigator |
This one was also resolved by |
🎯 @eVoloshchak, thanks for reviewing and testing this PR! 🎉 An E/App issue has been created to issue payment here: #29564. |
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
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
Performance Comparison Report 📊Significant Changes To Duration
Show details
Meaningless Changes To DurationShow entries
Show details
|
@Expensify/mobile-deployers 📣 Please look into this performance regression as it's a deploy blocker. |
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.84-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.84-10 🚀
|
🚀 Deployed to staging by https://github.com/Julesssss in version: 1.3.85-0 🚀
|
🚀 Deployed to production by https://github.com/francoisl in version: 1.3.85-4 🚀
|
@@ -1448,6 +1449,9 @@ function addPolicyReport(policyID, reportName, visibility, policyMembersAccountI | |||
|
|||
// The room might contain all policy members so notifying always should be opt-in only. | |||
CONST.REPORT.NOTIFICATION_PREFERENCE.DAILY, | |||
'', | |||
'', | |||
welcomeMessage, |
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 #29612
We didn't ensure that the special characters (like <
) are HTML-escaped
<SelectionList | ||
sections={[{data: sectionsData}]} | ||
onSelectRow={onItemSelected} | ||
initiallyFocusedOptionKey={currentValue} |
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.
Wrong value was used here. It should be selectedItem.value
Later it caused Workspace selector doesn't highlight when selected
This is basically a copy of #27836 with one change that is
behavior="padding"
for the keyboardAvoidignView to fix this issue #28950Details
You'll get the "too many auth calls" alert for this, due to the welcome message being added as an NVP. @jasperhuangg let me know how you want to handle this, whether we just add it to the list of exceptions, or if we need to somehow roll it into Auth. For local testing, please add the command to the list of exceptions otherwise you'll get an error when you try to create the room and won't be able to test. Staging server should be fine.
This also covers a bit of a refactor for the room creation page, using push-to-page selectors. Since we didn't really have a generic one of these before for use in forms, I created one. It takes a list of items (with values and labels) and will make them into one of these pop up selection modals. Handles display of the selected item, and offers an option for an initial placeholder (though you can't get back to it afterwards, which isn't an issue here since selections are required for all).
I'm not 100% confident on the name of the new component, so any suggestions are welcome.
Lastly, I also had to update the NewChatSelectorPage to include
withOnyx
because otherwise it wasn't actually able to access the betas prop, but now it works.Fixed Issues
$ https://github.com/Expensify/Expensify/issues/316189
$ #28950
PROPOSAL: N/A
Tests
Offline 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
Uploading web.mp4…
Mobile Web - Chrome
I can't get this to login, on
main
either, so I'm forgoing this. Some combo of Bali + ngrok + cloudflare + etc. etc.. If you could double test this it would be appreciated!Mobile Web - Safari
mWebSafari.mp4
Desktop
desktop.mp4
iOS
ios.mp4
Android
This is coming! Taking forever to build so I wanted to get a head start.