chats 1-1 reviews #17253
Merged
chats 1-1 reviews #17253
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Jenkins BuildsClick to see older builds (104)
|
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
7 times, most recently
from
fa8a9b7 to
9ff10bf
Compare
jo-mut
requested review from
OmarBasem,
J-Son89,
ajayesivan and
smohamedjavid
OmarBasem
reviewed
Sep 14, 2023
| @@ -48,7 +48,7 @@ | |||
| [theme window-height sheet-height {:keys [top]}] | |||
| {:position :absolute | |||
| :bottom 10 | |||
| :max-height (- window-height sheet-height top) | |||
| :max-height (- window-height sheet-height top 10) | |||
There was a problem hiding this comment.
I think this 10 need to be applied for Android only
There was a problem hiding this comment.
I checked with iOS and it seems necessary too to achieve the same design
There was a problem hiding this comment.
Also looks a bit like a magic number. Maybe some constant needs to be introduced or at least some explanation?
There was a problem hiding this comment.
Also looks a bit like a magic number. Maybe some constant needs to be introduced or at least some explanation?
@alwx I have added a comment on the code to explain how we arrive at that value. The selected item can only have a max-height of the remaining part of the screen not occupied by the bottom-sheet. We arrive at getting by: window-height minus (bottom-sheet-height + (:top insets) + margin-bottom). We minus the margin-bottom as its part of the height not occupied by the bottom-sheet
(- window-height sheet-height top bottom-margin)
alwx
requested changes
Sep 14, 2023
| @@ -48,7 +48,7 @@ | |||
| [theme window-height sheet-height {:keys [top]}] | |||
| {:position :absolute | |||
| :bottom 10 | |||
| :max-height (- window-height sheet-height top) | |||
| :max-height (- window-height sheet-height top 10) | |||
There was a problem hiding this comment.
Also looks a bit like a magic number. Maybe some constant needs to be introduced or at least some explanation?
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
2 times, most recently
from
5d198a2 to
780d097
Compare
ajayesivan
reviewed
Sep 15, 2023
| :bottom 10 | ||
| :max-height (- window-height sheet-height top) | ||
| :bottom 8 | ||
| ;; we minus 8 on the max-height since the bottom has a margin of 8 |
There was a problem hiding this comment.
How about using a variable for bottom instead of hardcoding 8 in both spots? This way, we won't need this comment, and it'll keep our UI safe in case we tweak bottom down the road.
ajayesivan
approved these changes
Sep 15, 2023
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
3 times, most recently
from
cff06f2 to
e9523ce
Compare
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
from
3c0adff to
6590d50
Compare
67% of end-end tests have passedFailed tests (14)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (29)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
Thanx for the PR @jo-mut. I have checked the PR and frankly speaking I was not able to see any difference compared to nightly in terms of described areas of fixes. Let's ask @Francesca-G for review. |
|
@pavloburykh we're currently working on the 1.25 build review and we have a lot of comments for 1-1 chats. That should be all ready by Monday, so should we wait for that big review to be ready or does it make sense to review this now? :) |
Thank you @Francesca-G! This PR provides some fixes based on previous feedback (1.24 release review). So I would appreciate if you provide feedback on this PR exclusively in terms of these issues [#17252], [#17251], [#17250], [#17249] Based on 1.25 review we will create new umbrella issues which will be fixed in separate PRs. Thank you! |
|
Thank you @Francesca-G! |
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
3 times, most recently
from
54ff783 to
0b68cc4
Compare
@Francesca-G @pavloburykh I have addressed the design feedback on the margins. Take a look. Thank you |
Once @Francesca-G approves the fix PR is ready for merge @jo-mut |
Francesca-G
reviewed
Oct 4, 2023
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
from
0b68cc4 to
79fb829
Compare
@Francesca-G thanks for these comments. I have addressed them if you could have another look just to confirm |
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
2 times, most recently
from
73407ba to
f60db7a
Compare
Francesca-G
approved these changes
Oct 5, 2023
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
from
f60db7a to
ef9ac02
Compare
jo-mut
force-pushed
the
bug/chats-1-1-reviews
branch
from
ef9ac02 to
9d02e62
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes [#17252], [#17251], [#17250], [#17249]