Implement collectible image preview #18449
Merged
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 (40)
|
vkjr
force-pushed
the
collectible-preview
branch
2 times, most recently
from
8633105 to
8f6f2f2
Compare
vkjr
requested review from
OmarBasem,
shivekkhurana,
FFFra,
briansztamfater and
J-Son89
vkjr
force-pushed
the
collectible-preview
branch
from
8f6f2f2 to
ea7bf1e
Compare
OmarBasem
reviewed
Jan 12, 2024
Comment on lines
+105
to
+111
| :image-width 300 ; collectibles don't have | ||
| ; width/height but we need | ||
| ; to pass something | ||
| :image-height 300 ; without it animation | ||
| ; doesn't work smoothly and | ||
| ; :border-radius not | ||
| ; applied |
There was a problem hiding this comment.
These values are used in zooming animations. Have you tested how does zooming look.
Instead of using an arbitrary value we could use the device's width as the image dimensions.
There was a problem hiding this comment.
No, I didn't find any problems with resizing.
Good idea, will try this!
There was a problem hiding this comment.
I tried using device dimensions and that resulted in wrong scaling and strange animation
vkjr
force-pushed
the
collectible-preview
branch
from
ea7bf1e to
51cac35
Compare
J-Son89
reviewed
Jan 17, 2024
| @@ -1,4 +1,4 @@ | |||
| (ns status-im.contexts.chat.messenger.lightbox.animations | |||
| (ns status-im.common.lightbox.animations | |||
J-Son89
reviewed
Jan 17, 2024
| (fn [{:keys [db]} [value]] | ||
| {:db (assoc db :lightbox/scale value)})) | ||
|
|
||
| (rf/reg-event-fx :chat.ui/share-image |
There was a problem hiding this comment.
:chat.ui/share-image should this event be in this ns?
J-Son89
reviewed
Jan 17, 2024
| (fn [_ [uri]] | ||
| {:effects.lightbox/share-image uri})) | ||
|
|
||
| (rf/reg-event-fx :chat.ui/save-image-to-gallery |
vkjr
force-pushed
the
collectible-preview
branch
from
ebe7f8a to
4b1a894
Compare
J-Son89
approved these changes
Jan 19, 2024
| (:require | ||
| [quo.foundations.colors :as colors] | ||
| [react-native.platform :as platform] | ||
| [react-native.reanimated :as reanimated] | ||
| [status-im.contexts.chat.messenger.lightbox.constants :as c])) | ||
| [status-im.common.lightbox.constants :as c])) |
There was a problem hiding this comment.
I see that sometimes this namespace is imported as c, and sometimes is imported as constants. We should unify the naming IMO.
|
|
||
| (defn convert-message-to-lightbox-image | ||
| [{:keys [timestamp image-width image-height message-id from content]}] | ||
| (let [[primary-name _] (rf/sub [:contacts/contact-two-names-by-identity from])] |
There was a problem hiding this comment.
Not sure an utils function should susbcribe, I think they should be more pure, maybe it can be an additional parameter?
There was a problem hiding this comment.
To be honest, I see no convenient way to remove this subscription out of the function. It is used within the map in status-im.contexts.chat.messenger.messages.content.album.view and subscription itself relies on other subs, so can't be easily converted to usual function. I would prefer to leave it "as is" because it encapsulates all the work needed to convert message to image description.
briansztamfater
approved these changes
Jan 19, 2024
69% of end-end tests have passedFailed tests (12)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Expected to fail tests (3)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Passed tests (33)Click to expandClass TestCommunityMultipleDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
|
OmarBasem
approved these changes
Jan 22, 2024
vkjr
force-pushed
the
collectible-preview
branch
from
fc475e0 to
fd5e426
Compare
qoqobolo
force-pushed
the
collectible-preview
branch
from
fd5e426 to
779d936
Compare
vkjr
force-pushed
the
collectible-preview
branch
from
779d936 to
d38ed89
Compare
85% of end-end tests have passedFailed tests (4)Click to expandClass TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Expected to fail tests (3)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Passed tests (41)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestActivityMultipleDevicePRTwo:
|
50% of end-end tests have passedFailed tests (2)Click to expandClass TestDeepLinksOneDevice:
Passed tests (2)Click to expandClass TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
|
|
@vkjr thanks for your work! PR |
vkjr
force-pushed
the
collectible-preview
branch
from
d38ed89 to
b53d95d
Compare
|
@qoqobolo, I'd leave this for the followup if you don't mind, wdyt? |
|
@vkjr sure, would you mind creating an issue for it, please? I also noticed some weird behavior with a couple of collectibles, but I think we'll double check that once #18577 is merged. Please, send it to design review before merging if you think it's needed. |
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 #17326
Summary
The lightbox screen is a screen for pictures preview.
Before it was used only for previewing sent photos in messages with images.
In the current PR it was untied from message data structures and moved to commons to be reused for previewing collectibles.
Previewing collectible image was made with this screen
Testing notes
Impacted area is image preview in messages and collectible preview
Testing steps
status: ready