Skip to content
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] My profile edit and share screens #8069

Closed
flexsurfer opened this issue Apr 29, 2019 · 39 comments · Fixed by #8722
Closed

[Profile] My profile edit and share screens #8069

flexsurfer opened this issue Apr 29, 2019 · 39 comments · Fixed by #8722
Labels
feature feature requests

Comments

@flexsurfer
Copy link
Member

flexsurfer commented Apr 29, 2019

Implement new UI for user name

image

image

Implement share icon and screen
image

Acceptance criteria:

  1. pixel perfect
  2. new name UI and icons should be introduced
  3. it should be possible to share
  4. it should be possible to edit a picture

Figma:
https://www.figma.com/file/TNCyHKtR3sx5EL6YznFWUa4O/Profile?node-id=510%3A3116

@flexsurfer flexsurfer mentioned this issue Apr 29, 2019
5 tasks
@errorists
Copy link
Contributor

@flexsurfer if you need help visualising the top bar's scroll behaviour or the animation for sharing, this prototype can help 👉 https://framer.cloud/AvjzY/

@flexsurfer
Copy link
Member Author

nice, thanks!

@flexsurfer flexsurfer added the feature feature requests label May 13, 2019
@StatusWrike
Copy link

➤ Julien Eluard commented:

Estimated to 2 days of work

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


This issue now has a funding of 300.0 DAI (300.0 USD @ $1.0/DAI) attached to it.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Workers have applied to start work.

These users each claimed they can complete the work by 12 months from now.
Please review their action plans below:

1) x5engine has applied to start work (Funders only: approve worker | reject worker).

I would love to build this UI screens and complete this before 2 days.

Some guidance over which file and folder to work on.

thanks

Learn more on the Gitcoin Issue Details page.

@gitcoinbot
Copy link

@tbenr Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@tbenr Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@tbenr
Copy link
Contributor

tbenr commented Jul 15, 2019

@gitcoinbot still finishing #8071 then i'll move on this

@gitcoinbot
Copy link

@tbenr Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

@tbenr Hello from Gitcoin Core - are you still working on this issue? Please submit a WIP PR or comment back within the next 3 days or you will be removed from this ticket and it will be returned to an ‘Open’ status. Please let us know if you have questions!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@tbenr due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

1 similar comment
@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


@tbenr due to inactivity, we have escalated this issue to Gitcoin's moderation team. Let us know if you believe this has been done in error!

  • reminder (3 days)
  • escalation to mods (6 days)

Funders only: Snooze warnings for 1 day | 3 days | 5 days | 10 days | 100 days

@rachelhamlin
Copy link
Contributor

FYI to anyone checking in: we'll have @bitsikka take over on this issue while @tbenr is away. Looking for @StatusSceptre's help in reassigning it on gitcoin, but the issue is claimed now.

@tbenr
Copy link
Contributor

tbenr commented Jul 26, 2019

@rachelhamlin I released the issue in gitcoin. I think @bitsikka can tale over now

@gitcoinbot
Copy link

gitcoinbot commented Jul 26, 2019

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work has been started.

These users each claimed they can complete the work by 10 months, 2 weeks from now.
Please review their action plans below:

1) bitsikka has been approved to start work.

I think top bar's scroll behavior can be achieved using onViewableItemsChanged prop
other challenges I'm sure I can figure out

Learn more on the Gitcoin Issue Details page.

@bitsikka
Copy link
Contributor

@rachelhamlin I released the issue in gitcoin. I think @bitsikka can tale over now

done!

thanks! @tbenr

@bitsikka
Copy link
Contributor

I had my build system broken for several days and fixed it yesterday.

Starting with this one (as opposed to #8070) because both are profile related issues, and we might need a fundamental change(replace) the use of scrollView to flatList to get onViewableItemsChanged(it fires with list items coming in and going out of viewable window - perfect) to achieve this effect https://framer.cloud/AvjzY/; And that change would affect implementation for #8070.

@rachelhamlin
Copy link
Contributor

Sounds good to me @bitsikka, thanks!

@bitsikka
Copy link
Contributor

bitsikka commented Aug 1, 2019

I had a slight interruption but I'm back on this.

  • Had to figure out how the chat key/qr share is different from wallet address/qr share(which already exists - which makes it that much easy). Basically copying the wallet address share and appropriating it for chat-key.
  • figuring out how ens name for wallet/chat key part
  • Found an edge-case bug related to touch/drag selecting/copying which I will report later

@bitsikka
Copy link
Contributor

bitsikka commented Aug 1, 2019

On that note, in the existing implementation of sharing chat key as modal screen, there is show-tribute-to-talk-warning? condition for visibility of said warning; But, new design in figma does not reflect that part. I am assuming that will come later and leaving it out as I port modal to popover design.

Also of note is that Figma design asks for tap on list item to copy, whereas existing functionality, and simple existing-native-support for the functionality, works with lang tap to copy only. Going with simple and lean as it was before.

Except that popover behaves differently than modal in that it can be dismissed much easily - tapping area for dismissal of popover is like 99% of the screen - as opposed to practically opposite in modal.

All that makes it hard/maybe-frustrating for user to get long-press to copy 🤔

@rachelhamlin
Copy link
Contributor

rachelhamlin commented Aug 1, 2019

@bitsikka ah yes - that would be correct. Please leave out the TtT bit.

Hmmm cc @errorists so he's aware of tap vs. long press. Was tap on list item to copy a request for a new behavior?

@errorists
Copy link
Contributor

@bitsikka hey, what exactly is stopping us from evoking the Copy menu via a regular tap? it's about discoverability, most of the time you'd try to tap first rather than long-pressing onto something. You mention that 99% of the screen area can be used to dismiss a popover, surely that's an exaggeration, it's specifically tapping outside of it and with these, the popover takes more than half easily :)

@bitsikka
Copy link
Contributor

bitsikka commented Aug 1, 2019

Thanks @rachelhamlin for making the key concern clear :)

@errorists there is no api for text or text input component to trigger the copy context menu programmatically in react-native that I know of. We'd have to fork the component out of react-native and implement it natively. There are some third party components that could be employed but maybe overkill.

I think what can be done is that we show a tooltip(the kind used to show error message elsewhere in text-input component) instead, as a visual cue, to inform user that the key has been copied to clipboard.

Re: 99% area for dismissing popover - I was simply copying the example of sharing wallet-address in current nightly/develop where this is true(please try it out) - in which case this would be a bug that needs fixing for wallet-address share(or maybe/probably popover implementation in general).

@bitsikka
Copy link
Contributor

bitsikka commented Aug 1, 2019

I think what can be done is that we show a tooltip(the kind used to show error message elsewhere in text-input component) instead as a visual cue to inform user that the key has been copied to clipboard.

I mean for this - the functionality itself "copy on tap" is not the problem - hardcoded behavior of copy context-menu upon long-press is the problem to replicate "on single tap"

@errorists
Copy link
Contributor

Ok, let's do it via long press. I've seen wallets made with React Native (Rainbow) where regular press -> context menu works, so I never thought this might require going through so many hoops.
And good lord, how did that wallet popover get through QA 😬 it definitely shouldn't be the case, tapping inside should be ignored. Thanks for mentioning that!

@bitsikka
Copy link
Contributor

bitsikka commented Aug 1, 2019

I've seen wallets made with React Native (Rainbow) where regular press -> context menu works, so I never thought this might require going through so many hoops.

Interesting! I will check out how rainbow does it and try to replicate it - having to long press actually bothered me very much in the first place to bring it up

@bitsikka
Copy link
Contributor

bitsikka commented Aug 1, 2019

@errorists turns out Rainbow which is iOS only app, uses react-native-tooltip - a third party component which is also iOS only :(

leaning towards using react-native-tooltip for iOS, with react-native built-in long-press version for Android for now, but not sure.

@bitsikka
Copy link
Contributor

bitsikka commented Aug 1, 2019

Now I'm thinking, it might be good idea and less over-kill to replicate platform specific fake copy context-menu/tool-tip with a custom non-native component. I don't think something "so simple" has to be natively implemented.

@errorists
Copy link
Contributor

bitsikka

ok how about something simple like showing a tooltip for a second after tap? for animated values are opacity 0 to 1 to 0 and translateY 10 to 0 to 10, duration 140ms ease.

@bitsikka
Copy link
Contributor

bitsikka commented Aug 2, 2019

ok how about something simple like showing a tooltip for a second after tap? for animated values are opacity 0 to 1 to 0 and translateY 10 to 0 to 10, duration 140ms ease.

@errorists Sounds good! - that idea did cross my mind

Then, I also found https://anchorchat.github.io/anchor-ui-native/#/context-menu which is implemented as a regular(non-native) component just like I suspected there would be. Existing tooltip, this component, and the idea you just suggested are/can-be implemented simply without native coding.

This idea is actually less step, clean, and simple; therefore better ux - so going with it!

@bitsikka
Copy link
Contributor

bitsikka commented Aug 2, 2019

I found a bug in ENS registration where it checks for already owned name. I needed ENS name to test the name UI part and tried to get already owned name registered in the app - but it failed to recognize I am the owner of the name even though I did own the name.

Turns out comparision of resolved name to wallet address in app failed because of difference in eip55 checksum. I got it to recognize my name as owned, by adding eip55/address->checksum on top of ethereum/normalized-address during comparision.

Also of note is that [:app-db :multiaccounts :address] stored is not eip55 checksummed address rather than the address resolved by the contract.

This is probably already known, or, there are improvements to ENS registration process coming, in which it will be resolved. Otherwise, as it exists, why would I have to do the registration process for already owned name and pay(or seem to pay) for gas? 🤔 among other problems.

@errorists
Copy link
Contributor

Cool! just fyi, we will need a robust tooltip component in the future once we'll finally start working on stuff like message reactions

reactions

@bitsikka
Copy link
Contributor

bitsikka commented Aug 2, 2019

Cool! just fyi, we will need a robust tooltip component in the future once we'll finally start working on stuff like message reactions

Cool! I remember seeing those 😄

in which case a native implementation would probably be the way to go. But I'll keep that in mind. Tooltip already exists for error messages - will take a look at its implementation too. I think better to call this context-menu.

@rachelhamlin
Copy link
Contributor

Oh dear. Glad you caught that ENS bug and the source of it too @bitsikka. I'll capture it in an issue--it didn't come up during QA of ENS name registration & verification.

@rachelhamlin
Copy link
Contributor

Hey @bitsikka! Doing some bounty management - you're on this one + #8070. Should we expect to review anything this week?

@bitsikka
Copy link
Contributor

bitsikka commented Aug 6, 2019

@rachelhamlin
I'm close to getting this one done to the point of being reviewed.

  • got the address sharing part done
  • working on profile pic/name/shadow/separator animation/transition - major-part/tricky-bit of this already figured out
  • porting bottom sheet remains

Planning on moving on to #8070 soon.

@rachelhamlin
Copy link
Contributor

Fab. Thanks for the update! Keep up the great work.

@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


Work for 300.0 DAI (300.0 USD @ $1.0/DAI) has been submitted by:

  1. @bitsikka

@StatusSceptre please take a look at the submitted work:


@gitcoinbot
Copy link

Issue Status: 1. Open 2. Started 3. Submitted 4. Done


The funding of 300.0 DAI (300.0 USD @ $1.0/DAI) attached to this issue has been approved & issued to @bitsikka.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature feature requests
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants