[#21571] ask user to update their profile #21862
Conversation
ulisesmac
requested review from
ilmotta,
seanstrom,
flexsurfer and
mohsen-ghafouri
Jenkins BuildsClick to see older builds (35)
|
ulisesmac
force-pushed
the
21571-ask-user-to-update-their-profile
branch
3 times, most recently
from
bff82e2 to
b36ecfe
Compare
|
@ilmotta @flexsurfer @seanstrom @mohsen-ghafouri the PR is ready, it'd be great to have your reviews! |
ulisesmac
changed the title
| @@ -13,45 +13,51 @@ | |||
| (update db :profile/profile dissoc :images))})) | |||
|
|
|||
| (rf/reg-event-fx :profile/edit-profile-picture-success | |||
| (fn [_ [images]] | |||
| (let [has-picture? (rf/sub [:profile/has-picture])] | |||
| (fn [{db :db} [show-toast? images]] | |||
There was a problem hiding this comment.
Maybe this first argument should also be a map?
I ask because it might read better to use a map instead of boolean, for example when we test the code:
[:profile/delete-profile-picture-success true]
would become
[:profile/delete-profile-picture-success {:show-toast? true}]
which I think is more descriptive of what's being enabled. thoughts?
There was a problem hiding this comment.
I agree with you @seanstrom I updated them now.
I passed single arguments because these events were only used once and internally as follow-up events.
Sometimes I'm a bit concerned about creating a new CLJS map and then destructuring it in another function when the API is not widely usedm I know it improves the reading but I think it's not that hard to understand. Usually for functions/events rarely used and with few args (e.g. 3) sometimes I prefer this approach.
| (def continue-button | ||
| {:width "100%" | ||
| :margin-left :auto | ||
| :margin-top (if platform/android? :auto 0) | ||
| :margin-right :auto}) | ||
|
|
||
| (def button-container | ||
| {:width "100%" | ||
| :padding-left 20 | ||
| :padding-right 20 | ||
| :padding-top 12 | ||
| :align-self :flex-end | ||
| :height 64}) |
There was a problem hiding this comment.
I'm not certain but I think we tend to avoid using "100%" (or percentages in general) for things like width or height. Is it possible we can achieve something similar by using flexbox via flex-direction: row or similar?
There was a problem hiding this comment.
I agree, I don't exactly know why this has been written like this, but I think we may revisit this implementation, as said in the description I cleaned a bit the namespace, extended the implementation but didn't try to rework it.
BTW, @ilmotta if this floating button were used in other places, IMO it's worth rewriting it in reanimated, to make it look more soft and responsive. We could address it along with the switch from ratoms -> use-state. I believe it's a low prio.
| (def scroll-view-height (reagent/atom 0)) | ||
| (def content-container-height (reagent/atom 0)) |
There was a problem hiding this comment.
Should we reset these values to zero when we unmount the component?
There was a problem hiding this comment.
It's probably too much to ask for the namespace to be rewritten to hooks since it's a large one, but that would be the best since that's the direction we are going.
There was a problem hiding this comment.
I think I prepared the component to be rewritten easier than before.
I didn't want to touch the implementation too much because these global ratoms are needed to control the floating button, that implementation was very buggy for a long period of time and in develop, AFAIK, it was stable. IMO that refactor should be done in a separate PR.
| (conj [:dispatch [:profile/delete-picture {:show-toast? false}]]) | ||
|
|
||
| :always | ||
| (conj [:dispatch-n [[:navigate-back] on-success]]))})) |
There was a problem hiding this comment.
:dispatch-n was made redundant by :fx. You can use:
(cond-> []
:always
(conj [:dispatch [:event-a]]
[:dispatch [:event-b]]))
;; => [[:dispatch [:event-a]]
;; [:dispatch [:event-b]]]There was a problem hiding this comment.
Thanks for noticing!
Fixed!
| (def scroll-view-height (reagent/atom 0)) | ||
| (def content-container-height (reagent/atom 0)) |
There was a problem hiding this comment.
It's probably too much to ask for the namespace to be rewritten to hooks since it's a large one, but that would be the best since that's the direction we are going.
ulisesmac
force-pushed
the
21571-ask-user-to-update-their-profile
branch
from
1ed9e31 to
e3c8123
Compare
100% of end-end tests have passedPassed tests (8)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
|
89% of end-end tests have passedFailed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (50)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestFallbackMultipleDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
100% of end-end tests have passedPassed tests (5)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
|
|
hi @ulisesmac thank you for PR. Question/Potential issue: should the 'introduce yourself' drawer be shown in case if user has edited his "display name" once via profile? Currently, it is shown Steps to reproduce:
Actual result:The 'Introduce Yourself' drawer appears, prompting the user to edit their profile again when the Edit button is tapped. edit.mp4 |
100% of end-end tests have passedPassed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
|
|
@ulisesmac this is the only issue I found. If this is identified as an issue and needs to be addressed, please let me know, and I’ll recheck the PR. Otherwise, the PR can be merged |
ulisesmac
force-pushed
the
21571-ask-user-to-update-their-profile
branch
from
e3c8123 to
cf5b9fc
Compare
|
@VolodLytvynenko The fix has been applied. Could please you re-test it again? Thank you! |
fixes #21571
Summary
This PRs adds a modal and a screen to ask users to update their profile.
Here are three videos showing the implementation, just as small demos:
https://github.com/user-attachments/assets/4d2e289f-79a4-41cc-b386-83d970de4d5c, https://github.com/user-attachments/assets/d59858d4-ca8f-4291-8597-0a0f8088a07d, https://github.com/user-attachments/assets/0fe3cf34-2cee-4138-9fa7-cf7b68ea1cea
Extra fixes
Screencast.From.2024-12-20.17-19-24.mp4
(
SkipandEdit Profilebuttons have the same size, the previous implementation would showSkipshorter)Review notes
I recovered the implementation we had for the previous onboarding and used it for the new "edit profile" screen, however, multiple fixes and extra validations had to be made. The code wasn't updated to use hooks, but it pass stable callbacks in the reagent way.
A further improvement to this screen and the color picker can be done, for example:
But we need to evaluate if we want to spend resources on it, since this screen is only showed once.
Testing notes
Please mainly test the
Continuebutton works as expected (is disabled or enabled) when a valid state is reached.Platforms
status: ready