Skip to content
This repository has been archived by the owner on May 13, 2024. It is now read-only.

Uphold wallet verification UI #497

Merged
merged 18 commits into from
Jul 23, 2019
Merged

Uphold wallet verification UI #497

merged 18 commits into from
Jul 23, 2019

Conversation

cg505
Copy link
Contributor

@cg505 cg505 commented Jun 11, 2019

Changes

Adds UI components (button at top, footer updates, verification panel screen) to indicate uphold wallet status.

See brave/brave-browser#4774.

Test plan

Link / storybook path to visual changes

Integration

  • Does this contain changes to src/components or src/

    • Will you publish to npm immediately after this PR, or wait until sometime in the future?
    • Incompatible API change to something existing (major version increase)
    • Adding new backwards-compatible functionality? (minor version increase)
    • Fixing a bug backwards-compatibly? (patch version increase)
  • Does this contain changes to src/features for brave-core?

    • Are there non backwards-compatible changes required for brave-core? Do not merge until brave-core PR is approvable. Link to brave-core PR:
    • Will you create brave-core PR to update to this commit after it is merged?
    • Wants uplift to brave-core feature branch?
      • When uplift-approved, merge to brave-core-0.VV.x feature branch
      • Create additional brave-core PRs for each feature branch to update commit

@cg505 cg505 force-pushed the uphold-verification-ui branch 3 times, most recently from cadd093 to f6afb83 Compare June 12, 2019 02:12
@@ -22,6 +22,8 @@ export interface Props {
id?: string
disabled?: boolean
icon?: {image: React.ReactNode, position: 'before' | 'after'}
beforeIcon?: React.ReactNode,
Copy link
Contributor

Choose a reason for hiding this comment

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

does the existing code not work for your case? in icon object you already have before and after options

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I need both before and after at the same time, which is why I changed this.

Copy link
Contributor

Choose a reason for hiding this comment

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

could we use existing icon object to achieve this and just extend it? I would like to avoid duplicating 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.

I couldn't think of a good way to extend the current icon prop to work for two different icons, so I thought this would be the cleanest solution. But if you have a good idea for how to do it inside icon that totally works.

Copy link
Contributor

Choose a reason for hiding this comment

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

cc @petemill for his thoughts

Copy link
Member

@petemill petemill Jun 17, 2019

Choose a reason for hiding this comment

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

This might need some design decision to clarify the change to the button in the system - @rossmoody? I'm not sure it's easily understandable for the user to grasp the meaning of the button when there are 2 icons.
image

If it isn't a button, but is some kind of 'status chip' then let's make a new component called that. It could for example have a prop which determines which icon is displayed called status which accepts 'success' / 'fail' / 'info'

...looking at it again, it really doesn't feel like it should be a button to me, since it doesn't perform an action. Instead it conveys information, and expands to show more detailed information (currently on click but may want to be on hover 1 day)

Copy link
Contributor

Choose a reason for hiding this comment

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

This is the first I'm seeing this and sadly I really don't have any insight as to how this works or what function it serves. @jenn-rhim will need to chime in on details surrounding this.

Copy link
Contributor

Choose a reason for hiding this comment

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

This is not the approved design. We removed this state. @NejcZdovc

Copy link
Contributor

@NejcZdovc NejcZdovc Jul 18, 2019

Choose a reason for hiding this comment

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

@petemill @rossmoody @jenn-rhim so even though that button provided above was not needed we still need it for this one:

image

@cg505 cg505 force-pushed the uphold-verification-ui branch from 647aaac to e296a0a Compare June 13, 2019 21:01
@cg505 cg505 requested review from NejcZdovc and ryanml June 13, 2019 21:02
@cg505 cg505 changed the title Uphold wallet verification Uphold wallet verification UI Jun 13, 2019
@cg505 cg505 marked this pull request as ready for review June 13, 2019 22:21
<StyledIcon>
<UpholdColorIcon />
</StyledIcon>
{'@' + username}
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's maybe do a quick check for username.length so we don't potentially display just an @

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If username.length === 0 what should we do? We probably shouldn't be showing the box at all in this case

Copy link
Contributor Author

@cg505 cg505 Jun 14, 2019

Choose a reason for hiding this comment

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

Actually we do this check in WalletWrapper, which will only display the box if username is truthy. and '' is falsy.

<StyledLink onClick={onVerifyClick}>
{getLocale('walletVerifyLink')}
</StyledLink>
{' '}
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this for stylistic purposes? Can we maybe accomplish this via CSS if so?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not really for stylistic purposes, there should just be a space between the link and the rest of the test and the space will get eaten up by JSX otherwise.

id
} = this.props

return (
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we break this up in to a couple more methods? Maybe one for footer and header

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@cg505 cg505 force-pushed the uphold-verification-ui branch from e296a0a to 0349d9c Compare June 14, 2019 18:45
@cg505 cg505 requested a review from ryanml June 14, 2019 20:32
@NejcZdovc
Copy link
Contributor

let's not merge until core PR is ready

@cg505 cg505 force-pushed the uphold-verification-ui branch 2 times, most recently from 68f5530 to 0861a67 Compare June 19, 2019 22:33
@cg505 cg505 requested review from petemill and rossmoody June 19, 2019 22:34
@cg505
Copy link
Contributor Author

cg505 commented Jun 19, 2019

Removed "Connected" state and rebased everything, ready for re-review.

@cg505 cg505 force-pushed the uphold-verification-ui branch 2 times, most recently from e2fd307 to 8ead10e Compare June 20, 2019 20:59
@NejcZdovc NejcZdovc force-pushed the uphold-verification-ui branch 3 times, most recently from 76d1def to cf1e3f5 Compare June 24, 2019 15:00
@cg505 cg505 force-pushed the uphold-verification-ui branch from cf1e3f5 to 2c9dd48 Compare June 24, 2019 20:17
@NejcZdovc NejcZdovc force-pushed the uphold-verification-ui branch from 26781b6 to 70b9b73 Compare June 25, 2019 13:04
@NejcZdovc NejcZdovc force-pushed the uphold-verification-ui branch 5 times, most recently from 8ad5ec1 to ba27198 Compare July 15, 2019 05:07
@NejcZdovc NejcZdovc force-pushed the uphold-verification-ui branch 3 times, most recently from 6ac97c2 to f854347 Compare July 18, 2019 19:40
@cg505 cg505 force-pushed the uphold-verification-ui branch from f854347 to 5875725 Compare July 19, 2019 23:36
@cg505 cg505 force-pushed the uphold-verification-ui branch from 5875725 to bc1f653 Compare July 19, 2019 23:41
@NejcZdovc NejcZdovc merged commit db9b8bd into master Jul 23, 2019
@NejcZdovc NejcZdovc deleted the uphold-verification-ui branch September 4, 2019 13:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants