Fix broken username and timestamp layout for long usernames #13174
Conversation
|
Hey @siddarthkay, and thank you so much for making your first pull request in status-react! ❤️ Please help us make your experience better by filling out this brief questionnaire https://goo.gl/forms/uWqNcVpVz7OIopXg2 |
Jenkins BuildsClick to see older builds (6)
|
| @@ -107,15 +107,17 @@ | |||
| [photos/member-photo from])]] | |||
| [react/view {:flex 1} | |||
| [react/view {:flex-direction :row | |||
| :justify-content :space-between} | |||
| :justify-content :space-between | |||
| :width :98% | |||
There was a problem hiding this comment.
could you please elaborate on this change ? does that mean all items will be 2% shorter ?
There was a problem hiding this comment.
what of there will be case where we need 4% ?
There was a problem hiding this comment.
Hi @flexsurfer : In some android layouts as seen in #11543 The timestamps would go out of the visible screen, hence I decided to restrict the width of the parent view to 98%
There was a problem hiding this comment.
could you elaborate please why 98% ?
There was a problem hiding this comment.
hi :) and thank you for the contribution ;)
There was a problem hiding this comment.
Sure, the logic behind 98% of available width is that we leave out 2% space as a margin on the right hand side and we only occupy 98% of available space to display the username and timestamp, this ensures that an overflow does not result in data going out of the screen( as observed in #11543 )
If you look at the screenshot I have share in the comments below it would show how the UI now looks like.
There was a problem hiding this comment.
but that means that now timestamp is shifted and has a bigger margin, and I'm still not sure if all cases are covered or there still will be the case when data goes out of the screen, what i mean is that this fix is for this specific case, and is not a general solution, and also timestamp now is not on its place its shifted on 2% to the left
There was a problem hiding this comment.
@flexsurfer : Thanks for taking the time to let me know your thoughts on this!
setting a fixed width to the parent view combined with a word-break property should ensure that textual content within that view stays within the defined limits.
Reasoning behind setting a 98% width was to ensure that most android devices have different viewport sizes and we could account for that.
I have highlighted the entire width of the React View element in discussion here to show you how much space it takes up on the screen.
This bug also appears only in the specific case when someone has set a long username and when a user sets a max length nickname for someone in their contacts.
After re-creating the issue I tested my fix on it and only then I have raised a PR for this issue.
If you have any different approach in mind or any scenario in which this would fail please let me know of that use case and I will plan my solution around it.
There was a problem hiding this comment.
thank you for the detailed explanation
|
This is how it looks |
| @@ -107,15 +107,17 @@ | |||
| [photos/member-photo from])]] | |||
| [react/view {:flex 1} | |||
| [react/view {:flex-direction :row | |||
| :justify-content :space-between} | |||
| :justify-content :space-between | |||
| :width :98% | |||
There was a problem hiding this comment.
thank you for the detailed explanation
|
Hi @siddarthkay, thank you for the contribution and for the fix! ISSUE 1: Long nickname / 3-random name overlaps with timestampThe timestamp is no longer shifted off the screen in case of a long username, but they are overlapped now. The issue is only reproducible on Android.
|
|
@qoqobolo : Thanks for letting me know about your device configuration, I will attempt to reproduce and fix that as well |
|
@qoqobolo : fixed this issue and validated it by hard coding a really long username to test, now no matter how long of a username, the app would dedicate only 75% of available screen width and keep the remaining space for timestamp and user profile icon. |
|
Thanks @siddarthkay, looks great in the timeline now! Is it okay for you to fix it in the same PR? If it's out of the scope of this PR, that's totally fine, I'll create a separate issue in that case, just let me know. Steps:
|
90% of end-end tests have passedFailed tests (16)Click to expand
Passed tests (145)Click to expand
|
|
It was decided in DM that the last issue will be logged separately. @siddarthkay thanks again for your work! |
|
hey @siddarthkay could you please squash commits, thanks |
siddarthkay
force-pushed
the
bug/11543-timestamp-is-shifted-off-the-screen
branch
from
8802ffe to
4618799
Compare
|
@flexsurfer : Squashed |
flexsurfer
force-pushed
the
bug/11543-timestamp-is-shifted-off-the-screen
branch
from
4618799 to
b0cc30d
Compare
|
it has been merged, github is broken |
[comment]: Fixes #11543
Platforms
status: ready