Improve the floating shell button and fix its position in the screens #16981
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
Parveshdhull
commented
Aug 10, 2023
Parveshdhull
commented
Aug 10, 2023
Parveshdhull
commented
Aug 10, 2023
Jenkins BuildsClick to see older builds (58)
|
33% of end-end tests have passedFailed tests (29)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Passed tests (14)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
Parveshdhull
force-pushed
the
fix/floating-shell-button
branch
4 times, most recently
from
6a26708 to
daa3863
Compare
OmarBasem
approved these changes
Aug 11, 2023
Comment on lines
-32
to
-35
| insets (safe-area/get-insets) | ||
| extra-height (utils/calc-top-content-height reply edit) | ||
| neg-y (utils/calc-shell-neg-y insets maximized? extra-height) | ||
| y-container (reanimated/use-shared-value neg-y) |
There was a problem hiding this comment.
Checking if reply or edit not needed?
There was a problem hiding this comment.
As jump-to button is now part of sheet itself, it gets pushed up according to the size of sheet. We don't need to manually calculate position anymore.
pavloburykh
force-pushed
the
fix/floating-shell-button
branch
from
61b93b8 to
12b0d31
Compare
Parveshdhull
force-pushed
the
fix/floating-shell-button
branch
from
12b0d31 to
8a30722
Compare
40% of end-end tests have passedFailed tests (26)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Passed tests (17)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
|
|
@Parveshdhull
Is there any way to distinguish somehow the one which is visible? |
|
HI @Parveshdhull Thank you for PR. Here's a list of the issues that have been found: ISSUE 1: [IOS] Textbox disappears into background and keyboard can't be closed after pasting long textSteps to Reproduce:
Actual Result:The textbox containing the pasted long text disappears into the background, and the keyboard cannot be closed ios_issue.mp4Expected Result:The textbox should remain visible. ios_nightly.mp4ENV:
|
ISSUE 2: [iOS] Incorrect positioning of 'Jump To' button after tapping 'Scroll Down' ButtonSteps to Reproduce:
Actual Result:In some cases, the 'Jump To' button is displayed at the same level as a message after tapping the 'Scroll Down' button. jumptoisabove.mp4Expected Result:After tapping the 'Scroll Down' button, the 'Jump To' button should be positioned under the message with appropriate spacing. ENV:
|
Parveshdhull
force-pushed
the
fix/floating-shell-button
branch
from
c7fbfe4 to
5a44543
Compare
Parveshdhull
force-pushed
the
fix/floating-shell-button
branch
3 times, most recently
from
8a8a20a to
cf469b3
Compare
81% of end-end tests have passedFailed tests (8)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (35)Click to expandClass TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
|
|
hi @VolodLytvynenko, @churik. Thank you very much for testing the PR, both issues should be fixed now. UPD: By both issues I mean Issue 1 and e2e problem. |
|
hi @Parveshdhull thank you for PR. No issues from my side |
Francesca-G
approved these changes
Aug 21, 2023
There was a problem hiding this comment.
The positioning in the screens looks good to me, so I'm approving.
I'm adding the follow up required label because as you can see the button itself doesn't completely match the design.
Blue one is design, orange one is implementation: the button looks smaller and the text doesn't seem to match.
Feel free to solve it in a separate issue or do what works best in this case :)
|
Hi @Francesca-G, Thank you very much for testing the PR. The component doesn't match the design because of the issue in the react-native text component. Not sure if that issue is fixable, as the issue is closed after some attempts & inactivity. Also, this looks like a base issue that can also affect the design review process in other PRs, so maybe we can discuss this more in the next design meeting. For now, I reopened the base issue. Probably we don't need a follow-up issue, as it will report the same problem. wdyt? |
Parveshdhull
force-pushed
the
fix/floating-shell-button
branch
from
cf469b3 to
356e5b7
Compare
andresceballosm
pushed a commit
to andresceballosm/status-mobile
that referenced
this pull request
Aug 29, 2023
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 #16887
Testing request:
Please check jump-to button height in all screens (home, communities, messages etc.)
Known issue:
#16331 (comment)
status: ready