Skip to content
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

[MM-58561] Calls: Add stop recording confirmation; update call screen #8007

Merged
merged 6 commits into from
Jun 19, 2024

Conversation

cpoile
Copy link
Member

@cpoile cpoile commented Jun 11, 2024

Summary

call screen rec loading rec active new stop rec confirmation
IMG_1637 IMG_1638 IMG_1639 IMG_1640
Screenshot_20240611-175437 Screenshot_20240611-175443 Screenshot_20240611-175450 Screenshot_20240611-175455

Ticket Link

Checklist

  • Added or updated unit tests (required for all new features)
  • Has UI changes
  • Includes text changes and localization file updates
  • Have tested against the 5 core themes to ensure consistency between them.
  • Have run E2E tests by adding label E2E iOS tests for PR.

Device Information

  • Android: 13, Pixel 6
  • iOS: 16.5.1, iPhone 14

Release Note

Calls: Added stop recording confirmation; redesigned the header of the call screen to better display recording badge

@cpoile cpoile added 2: Dev Review Requires review by a core commiter 2: UX Review Requires review by a UX Designer labels Jun 11, 2024
@abhijit-singh abhijit-singh added the Build Apps for PR Build the mobile app for iOS and Android to test label Jun 12, 2024
app/products/calls/alerts.ts Outdated Show resolved Hide resolved
Copy link

@abhijit-singh abhijit-singh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks awesome @cpoile! Thank you! A couple of minor things I noticed but they aren't blockers:

  1. The call title and the time are slightly misaligned. The title probably needs to be moved down a bit so that they are in the same line. It is more visible on iOS than on Android.
image
  1. The minimize icon needs to be moved to the right to match the spacing on the left and right of the header content. Probably needs to be moved by about 12pts.
image
  1. Not directly related to this PR, but the icon on the Stop recording menu item should also be red instead of the default grey.
image

Copy link
Contributor

@streamer45 streamer45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice!

@streamer45 streamer45 removed the 2: Dev Review Requires review by a core commiter label Jun 17, 2024
@cpoile
Copy link
Member Author

cpoile commented Jun 18, 2024

@abhijit-singh Can you take a look now? I made some changes: the left and right padding stays at 24 now. When the rec badge pops in, we increase the left and right width (but keep the collapse icon in the same place), which keeps the center title in the same position but reduces its width symmetrically. This way we can keep left/right balance, but maximize the center title size. Let me know what you think? We can go back to the original if you would prefer the time to not change (but then the center title will always be smaller).

@abhijit-singh abhijit-singh added Build Apps for PR Build the mobile app for iOS and Android to test and removed Build Apps for PR Build the mobile app for iOS and Android to test labels Jun 19, 2024
@abhijit-singh
Copy link

Thanks @cpoile! Looks great visually now!

@cpoile cpoile added 4: Reviews Complete All reviewers have approved the pull request and removed Build Apps for PR Build the mobile app for iOS and Android to test 2: UX Review Requires review by a UX Designer labels Jun 19, 2024
@cpoile cpoile merged commit 53b0110 into main Jun 19, 2024
20 checks passed
@cpoile cpoile deleted the MM-58561-rec-end-confirmation branch June 19, 2024 21:01
@amyblais amyblais added this to the v2.19.0 milestone Jun 19, 2024
fewva pushed a commit to fewva/mattermost-mobile that referenced this pull request Jul 4, 2024
@amyblais amyblais added the Docs/Needed Requires documentation label Jul 4, 2024
@cwarnermm cwarnermm added Docs/Not Needed Does not require documentation and removed Docs/Needed Requires documentation labels Jul 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request Docs/Not Needed Does not require documentation release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants