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-57267] Calls: Update post UI #7965

Merged
merged 6 commits into from
May 24, 2024
Merged

[MM-57267] Calls: Update post UI #7965

merged 6 commits into from
May 24, 2024

Conversation

cpoile
Copy link
Member

@cpoile cpoile commented May 22, 2024

Summary

iOS Android
IMG_089B269E109B-1 Screenshot_20240522-151332
IMG_E261F27E2C8A-1 Screenshot_20240522-151319

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.

Device Information

  • Android: 13, Pixel 6
  • Android: 13, Galaxy Tab s7+
  • iOS: 16.5.1, iPhone 14

Release Note

Calls: Update call post UI

@cpoile cpoile added 2: Dev Review Requires review by a core commiter 2: UX Review Requires review by a UX Designer labels May 22, 2024
@@ -363,7 +363,7 @@ export const canEndCall = async (serverUrl: string, channelId: string) => {
return false;
}

return isSystemAdmin(currentUser.roles) || currentUser.id === call.ownerId;
return isSystemAdmin(currentUser.roles) || currentUser.id === call.hostId;
Copy link
Contributor

Choose a reason for hiding this comment

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

is this a breaking change?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nope, we've sent hostId since 2022-11.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, running this against a previous plugin would make it fail because the backend check would be on the owner ID. Not overly worried though.

width: 0,
height: 2,
},
shadowRadius: 3,
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems like the figma values for shadows aren't accurate. Maybe the react native values don't map properly or something.

I played around with the values and this seemed to get things closer between android and iOS and inline with the design.

shadowColor: 'rgba(0,0,0,0.32)',
shadowOffset: {
    width: 0,
    height: 1,
},
shadowOpacity: 0.24,
shadowRadius: 1,
elevation: 2,

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, you're right, looks better. React native must do shadows a bit differently. Updated to 1.

Copy link
Contributor

@matthewbirtch matthewbirtch left a comment

Choose a reason for hiding this comment

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

Looks SO much better now. Thanks @cpoile!

Just the minor nit on the shadows that I noted in the code.

@matthewbirtch matthewbirtch removed the 2: UX Review Requires review by a UX Designer label May 23, 2024
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.

Looks great!

@@ -363,7 +363,7 @@ export const canEndCall = async (serverUrl: string, channelId: string) => {
return false;
}

return isSystemAdmin(currentUser.roles) || currentUser.id === call.ownerId;
return isSystemAdmin(currentUser.roles) || currentUser.id === call.hostId;
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically, running this against a previous plugin would make it fail because the backend check would be on the owner ID. Not overly worried though.

@cpoile cpoile added 4: Reviews Complete All reviewers have approved the pull request and removed 2: Dev Review Requires review by a core commiter labels May 24, 2024
@cpoile cpoile merged commit 3f7fa8b into main May 24, 2024
7 checks passed
@cpoile cpoile deleted the MM-57267-calls-post-ui branch May 24, 2024 18:16
@amyblais amyblais added this to the v2.18.0 milestone May 24, 2024
larkox pushed a commit to larkox/mattermost-mobile that referenced this pull request May 30, 2024
* MM-58259 - allow current host to run /end call; fix deprecated test fns

* update call card UI

* update call ended icon

* make joinCall async; adding loading spinner to join/start button

* Revert "make joinCall async; adding loading spinner to join/start button"

This reverts commit bbb1366.

* shadowRadius 3 -> 1
@amyblais amyblais added the Docs/Needed Requires documentation label May 31, 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/Needed Requires documentation release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants