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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: fix reply border style #2197

Merged
merged 6 commits into from
Sep 4, 2023
Merged

Conversation

aharwood9
Copy link
Contributor

馃幆 Goal

Fix #1982 and the curve wasn't showing on Android at all (hard to tell due to MessageRepliesAvatars sitting over it)

馃洜 Implementation details

  • Added flexDirection: row to the message-replies container
  • Update the padding and margins to be nicely padded for both right and left alignment
  • Remove the transparent border colour as that meant it never showed on Android
  • Removed the Android specific code as that fixed the need to have a transparent colour.

馃帹 UI Changes

iOS

Before -
Simulator Screenshot - iPhone 14 Plus - 2023-07-24 at 18 14 24
Simulator Screenshot - iPhone 14 Plus - 2023-07-24 at 18 14 18

After -

Simulator Screenshot - iPhone 14 Plus - 2023-07-24 at 18 14 34
Simulator Screenshot - iPhone 14 Plus - 2023-07-24 at 18 14 31

Android

Before -

After -

馃И Testing

鈽戯笍 Checklist

  • I have signed the Stream CLA (required)
  • PR targets the develop branch
  • Documentation is updated
  • New code is tested in main example apps, including all possible scenarios
    • SampleApp iOS and Android
    • Expo iOS and Android

@aharwood9 aharwood9 changed the title Fix reply border fix: fix reply border style Jul 24, 2023
borderRightWidth: 0,
},
}),
borderRightWidth: 0,
Copy link
Member

Choose a reason for hiding this comment

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

If its 0, if there a need to have this property in the styles anymore?

borderLeftWidth: 0,
},
}),
borderLeftWidth: 0,
Copy link
Member

Choose a reason for hiding this comment

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

If its 0, if there a need to have this property in the styles anymore?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, the current structure first passes styles.messageRepliesCurve with a borderWidth: 1. Then, we remove the opposite side border within each rightMessageRepliesCurve or leftMessageRepliesCurve. I've attached an image if we just remove that line.

Screenshot 2023-09-03 at 14 38 34

However, if this is not ideal, we could remove the borderWidth: 1 from the styles.messageRepliesCurve and replace it with borderWithBottom: 1 and then have a left or right borderWidth with each replies curve. This should give the same impact.

@khushal87
Copy link
Member

Hey @aharwood9, thanks for fixing this and providing your valuable contributions. 馃檶馃徎

@aharwood9
Copy link
Contributor Author

@khushal87 - sorry one additional comment. Should there be a horizontal margin or not between the curves and the avatars? See the below images.

With margin -
Screenshot 2023-09-03 at 14 45 09

Without margin -

Screenshot 2023-09-03 at 14 45 01

Currently this PR has margin

@khushal87
Copy link
Member

Hey @aharwood9, good point. According to the design, there should not be a margin. Can you add the fix? Thanks 馃槃

@aharwood9
Copy link
Contributor Author

Hey @aharwood9, good point. According to the design, there should not be a margin. Can you add the fix? Thanks 馃槃

Done 馃憤 if you are happy with my reply to your comment about borderLeftWidth: 0 then this should be ready! 馃槃

@khushal87 khushal87 merged commit 0e84191 into GetStream:develop Sep 4, 2023
3 checks passed
@github-actions github-actions bot mentioned this pull request Sep 26, 2023
6 tasks
@stream-ci-bot
Copy link
Contributor

馃帀 This PR is included in version 5.18.0 馃帀

The release is available on:

Your semantic-release bot 馃摝馃殌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Message Reply Border incorrectly aligned on own messages
3 participants