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

feat(recipient): add new recipient picker #4433

Merged
merged 6 commits into from
Nov 6, 2023

Conversation

satish-ravi
Copy link
Contributor

Description

Implement new recipient item to match the new send recipient screen design

Test plan

Unit tests, manually by using the new RecipientItem

image image

Related issues

Backwards compatibility

  • The previous QuestionIcon included a question mark with a surrounding circle, but this was never used by any screen, so just this replaced with the icon needed for the recipient screen
  • ContactCircle component has a bunch of new props, with defaults that match existing behavior to keep it backwards compatible

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This duplicates some of the stuff in RecipientItem.tsx, originally tried to keep this in one component with a bunch of props, but that made the component hard to read with a lot of boolean props / expressions. Here's the diff between the two components: https://gist.github.com/satish-ravi/50b28cd1f7521a0dc7115ddad9aa1885


export default class QuestionIcon extends React.PureComponent<Props> {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This icon was no longer used (was also flagged by knip). Replaced with the new QuestionIcon

{showValoraIcon && (
<Logo
type={LogoTypes.LIGHT}
style={styles.valoraIcon}
Copy link
Contributor

Choose a reason for hiding this comment

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

type error here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this was because the celo branding was not supporting this prop and CI uses it. Fixed when I updated the celo branding to accept this prop

Copy link

codecov bot commented Nov 6, 2023

Codecov Report

Merging #4433 (c8e60a5) into main (c24cb9d) will decrease coverage by 0.01%.
The diff coverage is 97.91%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4433      +/-   ##
==========================================
- Coverage   85.05%   85.05%   -0.01%     
==========================================
  Files         708      710       +2     
  Lines       26310    26349      +39     
  Branches     3567     3576       +9     
==========================================
+ Hits        22379    22412      +33     
- Misses       3868     3874       +6     
  Partials       63       63              
Files Coverage Δ
branding/celo/src/icons/Logo.tsx 100.00% <100.00%> (ø)
src/components/ContactCircle.tsx 100.00% <100.00%> (ø)
src/icons/QuestionIcon.tsx 100.00% <100.00%> (ø)
src/recipients/RecipientItem.tsx 100.00% <ø> (ø)
test/values.ts 100.00% <100.00%> (ø)
src/recipients/RecipientItemV2.tsx 96.15% <96.15%> (ø)

... and 2 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c24cb9d...c8e60a5. Read the comment docs.

{showValoraIcon && (
<Logo
type={LogoTypes.LIGHT}
style={styles.valoraIcon}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah this was because the celo branding was not supporting this prop and CI uses it. Fixed when I updated the celo branding to accept this prop

fill={type === LogoTypes.COLOR ? '#5EA33B' : mainColor}
/>
</Svg>
<View testID={testID} style={style}>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

style prop was supported on the Valora branding Logo
https://github.com/valora-inc/valora-app-branding/blob/48a6e3363ad7c9befbf0567f3dc370a427fce87b/src/icons/Logo.tsx#L38

CI uses celo branding, this update matches the valora logo implementation (even though its not strictly required, because the final app build doesn't use this)

@@ -36,6 +36,7 @@ exports[`Avatar renders correctly with address and name but no number 1`] = `
"height": 40,
"width": 40,
},
undefined,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The undefined snapshot updates are because the borderColor style on the ContactCirle component

/>
<path
d="M15.4577 19.7369C16.1419 18.9077 16.6324 17.9361 16.8932 16.8932C17.9361 16.6324 18.9077 16.1421 19.7369 15.4579C19.699 16.6658 19.439 17.8563 18.9695 18.9698C17.8561 19.439 16.6656 19.6992 15.4577 19.7369ZM8.10687 8.10687C7.06397 8.36766 6.09239 8.85792 5.26318 9.54213C5.30108 8.33424 5.56108 7.14371 6.03055 6.03029C7.14397 5.56108 8.3345 5.30082 9.54239 5.26318C8.85818 6.09239 8.36766 7.06397 8.10687 8.10687Z"
<View>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This snapshot update is because of the celo Logo refactor to support style prop

@satish-ravi satish-ravi merged commit ac46f55 into main Nov 6, 2023
15 checks passed
@satish-ravi satish-ravi deleted the satish/act-952-recipient-item branch November 6, 2023 23:38
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.

2 participants