-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: Add visible to you text on ephemeral posts #8245
Conversation
Hello @tanmaythole, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. |
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.
Hi @tanmaythole -- thanks for your contribution. I only have 2 quick questions, but otherwise looks good to me.
@@ -39,18 +40,24 @@ const getStyleSheet = makeStyleSheetFromTheme((theme: Theme) => { | |||
header: { | |||
flex: 1, | |||
flexDirection: 'row', | |||
marginTop: 10, |
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.
Could you help me understand why the marginTop
addition is necessary?
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 mentioned in the ticket, the layout for system ephemeral post not placed as expected. But this change affects the other posts, will fix by adding condition specific to ephemeral post.
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.
Thank you for the contribution! Just one more comment.
@abhijit-singh Do you mind verifying the screenshot that it looks as you would expect? |
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.
Thanks @tanmaythole, looks good to me visually.
In the second screenshot, not sure how there are ephemeral posts from people with replies on them but I'm guessing that's just for testing purposes and won't actually ever happen/.
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.
Looks great. Thanks for contribution!
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.
@holopin-bot @tanmaythole Mattermost Most Valued Professional (MVP) |
Seems like there's something wrong with your config. See this page for more information. |
Summary
Added a visible to you text on ephemeral posts.
Ticket Link
Fixes mattermost/mattermost#28201
Checklist
E2E iOS tests for PR
.Device Information
This PR was tested on: Android Emulator Pixel 8a
Screenshots
Release Note