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

4127: send to address #4137

Merged
merged 12 commits into from
Nov 21, 2023
Merged

4127: send to address #4137

merged 12 commits into from
Nov 21, 2023

Conversation

L03TJ3
Copy link
Collaborator

@L03TJ3 L03TJ3 commented Nov 16, 2023

Description

It was already implemented, but not used. so here we enable the UI so that users can send their g$'s directly to an addres

  • - add flow to send via address
    finalize UI points:
  • - update paste icon with a new one (make fontello compatible)
  • - update send native flow
  • - general marging/padding

About # (link your issue here)
#4127

How Has This Been Tested?

Please describe the tests that you ran to verify your changes.

Checklist:

  • PR title matches follow: (Feature|Bug|Chore) Task Name
  • My code follows the style guidelines of this project
  • I have followed all the instructions described in the initial task (check Definitions of Done)
  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • I have added reference to a related issue in the repository
  • I have added a detailed description of the changes proposed in the pull request. I am as descriptive as possible, assisting reviewers as much as possible.
  • I have added screenshots related to my pull request (for frontend tasks)
  • I have pasted a gif showing the feature.
  • @mentions of the person or team responsible for reviewing proposed changes

Copy link

vercel bot commented Nov 16, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
good-dapp ✅ Ready (Inspect) Visit Preview 💬 Add feedback Nov 21, 2023 2:29pm
2 Ignored Deployments
Name Status Preview Comments Updated (UTC)
gooddollar-delta ⬜️ Ignored (Inspect) Visit Preview Nov 21, 2023 2:29pm
goodid ⬜️ Ignored (Inspect) Visit Preview Nov 21, 2023 2:29pm

@L03TJ3 L03TJ3 marked this pull request as ready for review November 17, 2023 10:34
@L03TJ3
Copy link
Collaborator Author

L03TJ3 commented Nov 17, 2023

@johnsmith-gooddollar @sirpy
left-over todo-s seem to be only ui (updating a svg icon compatibility with fontello, confirming copies with the rest)
technically, I currently can't think of changes needed

@@ -21,6 +21,7 @@ const InputText = ({
adornmentStyle,
adornmentColor,
adornmentDisabled = false,
iconAlignment = 'right',
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1, what have you decided about both paste + scan QR options ?
in this case you need to display 2 icon buttons somehow - need to discuss this

  1. you have repeated code. optimise it at leas by using useMemo:
const adornmentJsx = useMemo(() => showAdornment ? (
  <TouchableOpacity style={[styles.adornment, adornmentStyle]} disabled={adornmentDisabled} onPress={_onPress}>
   <Icon size={normalize(adornmentSize)} color={adornmentColor || inputColor} name={adornment} />
 </TouchableOpacity>
) : null, [styles, adornmentStyle, adornmentDisabled, _onPress, adornmentSize, adornmentColor, inputColor, adornment])

Comment on lines 80 to 84
{showAdornment && iconAlignment === 'left' && (
<TouchableOpacity style={[styles.adornment, adornmentStyle]} disabled={adornmentDisabled} onPress={_onPress}>
<Icon size={normalize(adornmentSize)} color={adornmentColor || inputColor} name={adornment} />
</TouchableOpacity>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{showAdornment && iconAlignment === 'left' && (
<TouchableOpacity style={[styles.adornment, adornmentStyle]} disabled={adornmentDisabled} onPress={_onPress}>
<Icon size={normalize(adornmentSize)} color={adornmentColor || inputColor} name={adornment} />
</TouchableOpacity>
)}
{iconAlignment === 'left' && adornmentJsx}

Comment on lines 93 to 96
{showAdornment && error !== '' && iconAlignment === 'right' && (
<TouchableOpacity style={[styles.adornment, adornmentStyle]} disabled={adornmentDisabled} onPress={_onPress}>
<Icon size={normalize(adornmentSize)} color={adornmentColor || inputColor} name={adornment} />
</TouchableOpacity>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{showAdornment && error !== '' && iconAlignment === 'right' && (
<TouchableOpacity style={[styles.adornment, adornmentStyle]} disabled={adornmentDisabled} onPress={_onPress}>
<Icon size={normalize(adornmentSize)} color={adornmentColor || inputColor} name={adornment} />
</TouchableOpacity>
{error !== '' && iconAlignment === 'right' && adornmentJsx}

disabled={loading}
{...props}
/>
{isSend && (
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

have you tested it with the native flow ? set REACT_APP_DELTA=true in Your local env, at dev instance you will be able to switch onto Goerli and select gEth to send. If you need some Goerli pls ask me

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes I did test, have not tested sending any funds yet tho. will do

@sirpy
Copy link
Contributor

sirpy commented Nov 19, 2023

@L03TJ3 make sure unit tests are passing

@johnsmith-gooddollar
Copy link
Collaborator

@L03TJ3 regarding input with 2 icons

  1. copy InputWithAdornment to the new component, call it InputWithAddons
  2. introduce new props
{
  prefixIcon: string | boolean // left-side icon name, false if hidden
  prefixDisabled: boolean
  prefixColor: string
  prefixStyle: object
  prefixIconSize: number
  onPrefixClick: Function

 // the same for the right-side
  suffixIcon: string | boolean
  suffixDisabled: boolean
  suffixColor: string
  suffixStyle: object
  suffixIconSize: number
  onSuffixClick: Function

}

3. rewrite InputWithAdornment to just render InputWithAddons passing adornment props as 'prefix' or 'suffix' ones (depending on showAdornment value)

@L03TJ3 L03TJ3 merged commit 0855c8e into master Nov 21, 2023
5 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants