-
Notifications
You must be signed in to change notification settings - Fork 988
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Profile] Other profile screen UI #8776
Conversation
Pull Request Checklist
|
Jenkins BuildsClick to see older builds (83)
|
677d0cd
to
08aaa52
Compare
08aaa52
to
a354322
Compare
@tbenr is this still wip? |
@flexsurfer yes, need to remove unused files and a couple of other things.. this evening i should finish. |
9accc93
to
58b1e5d
Compare
@flexsurfer I think we can start the review. |
58b1e5d
to
2076fc1
Compare
src/status_im/contact/core.cljs
Outdated
[{:keys [db] :as cofx} public-key] | ||
(fx/merge cofx | ||
{:db (update db :contacts/contacts dissoc public-key) | ||
:data-store/tx [(contacts-store/delete-contact-tx public-key)]})) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i believe we don't use :data-store/tx
for contacts anymore @cammellos could you assist, please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for sure we don't, the question is do we have a "shhext_deleteContact" rpc method to replace it? @cammellos
if we don't I'd say just add a TODO here that says the contact is not actually remove, because this is a UI PR and there is no point blocking it waiting for a core feature. Remove that :data-store/tx it the delete-contact-tx function won't exist once you rebase anyway.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i added a TODO
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we remove the contact completely (needs a new endpoint in status-go), or is it enough to remove the contacts/added
tag (does not need a new endpoint)?
If we delete it completely, it will come back (maybe not in the list if we are filtering by contact/added
tag), next contact update, so not sure how useful it is, but I don't have much context on this task.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose we just want to remove contacts/added, it removes the user from the contact list right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure about that, I would guess so as it should only show contacts that we added.
Essentially we can just remove the contact/added
tag and call save-contact
in status-im.data-store.contacts
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cammellos @yenda take a look to the latest push
|
||
(defn- contact->details | ||
"prepare a sequence of datails from a given contact" | ||
[contact] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could you elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@flexsurfer the function should "prepare" a list of details (accounts\wallet) from the current contact. Since i don't know how will actually wallet\accounts be stored in db for a given contact, I prepared a function for the purpose.
Currently it just create an array with current account.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
contact will always be a map, wallet and accounts will be part of it. I'd just remove this fn a use contact directly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yenda I missed this comment, later this afternoon i'll remove it.
2076fc1
to
36ff826
Compare
@flexsurfer i added the accessory goes off-screen. Any hint? |
36ff826
to
43db7dd
Compare
@tbenr let me chime in here. As a critical part of #8070 I am adjusting the
I am almost done with the PR for #8070 and will be pushing it soon |
Sorry for that, this time was about permissions due to branch not being in the repo. |
660c9bd
to
ae60247
Compare
@flexsurfer @churik @Serhy can we try to test again? |
@tbenr apologies for any delay - we're on our team offsite in Istanbul this week so will be responding a bit more slowly than usual. Would love to get this one finalized so we can pay you! Will see if we can prioritize it. |
Merging this will also help me with PRs I'm working on 🙌 :) |
@tbenr sorry for delay, we all on offsite in Istanbul. |
91% of end-end tests have passed
Failed tests (4)Click to expand
Passed tests (42)Click to expand |
@tbenr two small issues:
Thank you for hard work! |
@churik damn :) i'll fix it when back home... thanks for patient |
ae60247
to
e4660e8
Compare
@churik MAYBE HOPEFULLY this is the last time :) |
50% of end-end tests have passed
Failed tests (1)Click to expand
Passed tests (1) |
@tbenr :) And I believe we already got confirmation from @errorists that he is OK with new view. |
@churik ahh ok I reactivate it once back home. No problems:) |
e4660e8
to
dc2625f
Compare
@churik reenabled sheet on block, rebased too. |
@tbenr many thanks for your patience! |
Signed-off-by: yenda <[email protected]>
dc2625f
to
f707601
Compare
fixes: #8071
after #8069 as been merged I integrate my part with the work from the great @bitsikka
regarding large_toolbar:
large_toolbar
, decoupling styles too.large_toolbar
implementation on flat-list's on-scroll eventsthen i implemented the "other profile". It is still not supporting multiple details (main chat account is listed) but the basis for listing multiple accounts\wallets is there.
status: ready