Closed lauridskern closed 3 months ago
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31055/index.html | ❌ FAILED ❌ | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/31055/NewExpensify.dmg | https://31055.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
But wow yeah, web loads so fast that you can't even notice anything but a quick flash of green.
If I already had an app running in the background and I switch to it, I don't expect to see any kind of boot/loading animation.
I still think a big % of users are unaware of the difference other than apps sometimes taking longer to load.
Without the audio, I think that most people would notice the difference. It's very noticeable. But let's save that discussion for later. My wider issue is with the audio and delay
still issue
Thank you @aimane-chnaif for testing! The workaround was for the bug where there was a cutout in the splash screen where you could see the content through. That seems to be fixed now! In your case i think its that the video takes a tad longer loading (which could be optimized once we switch format). Also in your video we can see that while we already fade out the splash screen the video starts to play, which looks odd. I guess if the video hasn't started while we are already fading the splash screen out we just shouldn't show the video anymore. Will add a fix for that soon.
So, as a list of outstanding items i see:
Re: Keep audio or not, we do want to keep all four versions with audio, because the whole point of this test was to help us decide on the final brand mnemonic.
Let me know when you get through with the other items - I'd love to get this to a point where we can generate the QR codes and have people test this on their own devices. Again, I think they will come to the same conclusion that I have in that the start up sound is just not working and we should ditch the idea.
It would also be nice for that same crowd of testers to test the other UI sound effects we chatted about in Slack, too.
@shawnborton should we add the sound effects to this PR, or should it be separate? (I'd say it should be separate? But it could be based off this pr)
I think for testing purposes, ideally we’d be able to test both the opening animation PLUS ui sounds from the same build. This way we don’t need to ask people to download multiple different test apps. Let me know if you’re okay with that though!
On Fri, Jan 12, 2024 at 8:14 AM Hanno J. Gödecke @.***> wrote:
@shawnborton https://github.com/shawnborton should we add the sound effects to this PR, or should it be separate? (I'd say it should be separate? But it could be based off this pr)
— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/pull/31055#issuecomment-1888547847, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARWH5Q6LBUBZG3R5QJ5XU3YODPGFAVCNFSM6AAAAAA7CT3U4GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQOBYGU2DOOBUG4 . You are receiving this because you were mentioned.Message ID: @.***>
How is this one moving along?
I have read the CLA Document and I hereby sign the CLA
recheck
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31055/index.html | https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/31055/index.html | |
Desktop :computer: | Web :spider_web: | |
❌ FAILED ❌ | https://31055.pr-testing.expensify.com | |
The QR code can't be generated, because the Desktop build failed |
:eyes: View the workflow run that generated this build :eyes:
cc @Expensify/design in case you wanna test out our sound effects! Still some more work to be done before we distribute this one for more testing, see this thread for more context
Awesome! I tested it on Android and added feedback in that thread 👍
I can't seem to get the ad hoc iOS app installed. I get the error message This app cannot be installed because its integrity could not be verified.
But on web I am looooooving the "send" sound. Can't wait for more!
I can't seem to get the ad hoc iOS app installed. I get the error message
This app cannot be installed because its integrity could not be verified.
But on web I am looooooving the "send" sound. Can't wait for more!
Because your iPhone udid isn't registered. If you wanna test on iPhone, please ask in slack to get your iPhone registered for adhoc build.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31055/index.html | ❌ FAILED ❌ | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
❌ FAILED ❌ | https://31055.pr-testing.expensify.com | |
The QR code can't be generated, because the Desktop build failed |
:eyes: View the workflow run that generated this build :eyes:
Hmm iOS and Desktop failed, any ideas why? I'll try to trigger a new build though and see if that fixes anything.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31055/index.html | ❌ FAILED ❌ | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
❌ FAILED ❌ | https://31055.pr-testing.expensify.com | |
The QR code can't be generated, because the Desktop build failed |
:eyes: View the workflow run that generated this build :eyes:
Hmm iOS and Desktop failed, any ideas why? I'll try to trigger a new build though and see if that fixes anything.
@shawnborton very strange, I didn't modify any XCode-related things since last successful build (but iOS build fails because of ObjC code compilation issues). I'll have a look tomorrow why it's failing 👀
In a meantime - would it be possible to test Android and web? (because most likely bugs will be identical across all platforms).
Maybe we just need a fresh pull of main?
On Mon, Jan 22, 2024 at 11:06 PM Kirill Zyusko @.***> wrote:
Hmm iOS and Desktop failed, any ideas why? I'll try to trigger a new build though and see if that fixes anything.
@shawnborton https://github.com/shawnborton very strange, I didn't modify any XCode-related things since last successful build (but iOS build fails because of ObjC code compilation issues). I'll have a look tomorrow why it's failing 👀
In a meantime - would it be possible to test Android and web? (because most likely bugs will be identical across all platforms).
— Reply to this email directly, view it on GitHub https://github.com/Expensify/App/pull/31055#issuecomment-1904907937, or unsubscribe https://github.com/notifications/unsubscribe-auth/AARWH5WQKYIQDRVSFUX3NBLYP3PHTAVCNFSM6AAAAAA7CT3U4GVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSMBUHEYDOOJTG4 . You are receiving this because you were mentioned.Message ID: @.***>
@kirillzyusko @hannojg on web, I am still getting two simultaneous sounds when I receive a message:
Is it possible to not have both of these sounds play at the same time? Not sure what our options are there, but it definitely does not feel right.
The desktop build seemed to fail because of a timeout - should work next time we create a build I assume.
The iOS build seemed to fail due to the new library we introduced. Are we able to reproduce this build error when building for release mode @kirillzyusko ?
For the push notification, it should be possible to send a silent notification. We tried to make that happen from the code, but this might need to happen server side. Will figure it out with expensify crew on slack!
Sounds good, thanks for digging in!
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31055/index.html | https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/31055/index.html | |
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/31055/NewExpensify.dmg | https://31055.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
One thing I noticed is that when the task creator marks a task as done, the Success sound didn't play.
On that note, can you actually out in simple plain english all of the cases where sounds do play? This way we can evaluate if we want to make any adjustments or changes to the various scenarios.
Sure @shawnborton
I added sounds in next cases:
when the task creator marks a task as done, the Success sound didn't play
This is because we receive a different data shape and I thought it's not the case "when someone completes task", because I thought that "someone" in this context means "not you". I hope it makes sense what I'm saying 😅
I hope it makes sense what I'm saying 😅
Makes sense but I kind of think we'd just play Success regardless of who marks the task as done.
Another small one - using @here should do the same sound as a mention
Hmm it looks like if I am viewing a task (basically a thread) and someone sends me a message, I don't get the message received sound. Can you make sure the message received sound is playing for all room types? DMs, #rooms, workspace chats, reports, expenses, threads, tasks, etc - this should all work the same I would think.
Okay @shawnborton I'll fix it tomorrow. Just to sum it up:
@here
should also play sound (maybe you has also pre-defined words, such as @channel
etc.? I'm not very familiar with the app yet so decided to ask first)Thank you for your testing. I'll try to fix all these issues tomorrow 👀
whenever task gets completed we should play a sound (regardless of who marks the task as done)
Correct, whenever task is completed, let's do Success sound
using @here should also play sound (maybe you has also pre-defined words, such as @channel etc.? I'm not very familiar with the app yet so decided to ask first)
Fairly certainly we only have @here
(not @channel
) so let's only worry about that one
we should always play a sound when message arrives (doesn't matter on which screen is currently located)
Correct!
@shawnborton I fixed all these issues! Would you mind to test again and see if there is everything works as expected? (I hope it works as expected 😅 🤞)
Amazing! Going to trigger a new build now.
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31055/index.html | https://ad-hoc-expensify-cash.s3.amazonaws.com/ios/31055/index.html | |
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/31055/NewExpensify.dmg | https://31055.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
Nice, this is feeling pretty good! I think I'll distribute these for testing now.
One strange thing I noticed... if I change windows on my Mac away from the Expensify chrome window, then go back to that window, the first message I send does not have a sound. But the second and subsequent messages do have a sound. Any ideas there?
One strange thing I noticed... if I change windows on my Mac away from the Expensify chrome window, then go back to that window, the first message I send does not have a sound. But the second and subsequent messages do have a sound. Any ideas there?
@shawnborton I don't have any ideas 🤔 I'd suggest to keep it in mind and distribute the build to testers anyways. When we gather all bugs in one place I'll fix them all at once! How does it sound to you?
Works for me, will keep ya posted.
Okay so after some testing, we decided that we want to get rid of the start up sound/animation and just go back to our standard "E" icon. We can follow up later with the animated coin spinner or something different, but until then, let's just get rid of that part.
So if you don't mind, could you update this PR by reverting the start up animation/sound? Then we can have a test build that just has the UI sounds on it (please pull main when you get a chance, too). Thanks!
@shawnborton sure! Tomorrow I will:
Wonderful, thanks!
Let me know when this is ready for a new build!
@shawnborton sure! I merged upstream and reverted animated splash screen implementation - going to check the issue with missing sound now 👀
I'll let you know when I finish!
if I change windows on my Mac away from the Expensify chrome window, then go back to that window, the first message I send does not have a sound.
@shawnborton I can not reproduce the issue you've described 😔 I've tried to switch between tabs in Chrome as well as switching between windows (i. e. minimizing Chrome -> Finder -> Spotify -> Chrome) and I tried to combine these approaches, but I always hear first sound 🤔
Am I doing something wrong? Maybe you can send me a video and show your reproduction steps? Also which Chrome browser version are you using?
Hmm let's spin up a new test build and I'll try to capture it, but maybe it's nothing we need to worry about for now. Any more changes coming or should I trigger the build?
Any more changes coming or should I trigger the build?
No more upcoming changes - you can trigger a build 🙂
:test_tube::test_tube: Use the links below to test this adhoc build on Android, iOS, Desktop, and Web. Happy testing! :test_tube::test_tube: | Android :robot: | iOS :apple: |
---|---|---|
https://ad-hoc-expensify-cash.s3.amazonaws.com/android/31055/index.html | ❌ FAILED ❌ | |
The QR code can't be generated, because the iOS build failed | ||
Desktop :computer: | Web :spider_web: | |
https://ad-hoc-expensify-cash.s3.amazonaws.com/desktop/31055/NewExpensify.dmg | https://31055.pr-testing.expensify.com | |
:eyes: View the workflow run that generated this build :eyes:
Hmm, iOS failed again. I'll retrigger the build but you might need to merge main in (again)?
@shawnborton I merged everything few hours ago. I think we just should retry a job, because it failed due to connection problems:
Details
This PR implements sounds playing when message gets received (only when app is in foreground, i. e. not sounds for push notifications).
To play a sound I used
react-native-sound
( andreact-native-web-sound
for web, underhood it useshowler.js
). Initially I wanted to useexpo-sound
but was getting ridiculous errors and couldn't resolve them (on iOS and web).Fixed Issues
$ https://github.com/Expensify/App/issues/29835 PROPOSAL: N/A
Tests
@here
also should produce the sound);Offline tests
QA Steps
@here
also should produce the sound);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)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG)
)Avatar
is modified, I verified thatAvatar
is working as expected in all cases)Design
label so the design team can review the changes.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
Android: Native
https://github.com/Expensify/App/assets/22820318/ee35fde4-056c-458b-8d75-32b30580dd44Android: mWeb Chrome
iOS: Native
https://github.com/Expensify/App/assets/22820318/e328f648-697c-4c29-a552-eb462d9d988fiOS: mWeb Safari
MacOS: Chrome / Safari
https://github.com/Expensify/App/assets/22820318/a8ebbe88-deab-4134-a107-573b269faf3cMacOS: Desktop
https://github.com/Expensify/App/assets/22820318/3c3072d0-98dc-4afd-8401-40fb7fd86c97