Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Make Signing Requests more visible #7204

Merged
merged 8 commits into from
Dec 6, 2017
Merged

Make Signing Requests more visible #7204

merged 8 commits into from
Dec 6, 2017

Conversation

amaury1093
Copy link
Contributor

Fix #7192. Red label was not comprehensible to drive users to action, so introducing a more classic notification UX.

screen shot 2017-12-05 at 12 53 03 pm

screen shot 2017-12-05 at 12 52 43 pm

"View" button opens the Signer as before.

@amaury1093 amaury1093 added A0-pleasereview 🤓 Pull request needs code review. M7-ui labels Dec 5, 2017
@parity-cla-bot
Copy link

It looks like @amaurymartiny signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

<div className={ [styles.signerPending].join(' ') }>
<Icon name={ this.store.pending.length > 0 ? 'bell' : 'bell outline' } />
{this.store.pending.length > 0 &&
<Label floating color='red' size='mini' circular className={ styles.label }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we keep to an attribute per-line?

size='mini'
/>
</List.Content>
<Image avatar size='mini' verticalAlign='middle'>
Copy link
Contributor

Choose a reason for hiding this comment

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

As above, attribute per line

this.setState({ isOpen: false });
}

renderEtherValue (value) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This probably makes sense as a smaller component? (I know we have render* methods all-over, but it is a bit of something that says "this really should be a component"

}
</List>
) : (
<Container textAlign='center' fluid className={ styles.noRequest }>
Copy link
Contributor

Choose a reason for hiding this comment

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

Attribute per line


render () {
return (
<Popup
Copy link
Contributor

Choose a reason for hiding this comment

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

Generally this whole render is quite a beast - probably makes sense to try and condense it, maybe split at least the content bits (where we have a ?:) into seperate components?

@jacogr jacogr added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 5, 2017
@amaury1093
Copy link
Contributor Author

amaury1093 commented Dec 5, 2017

  • Fixed Jaco's comments
  • Added different description whether we're sending ETH, tokens, calling a method on a contract etc.

screen shot 2017-12-05 at 6 28 52 pm

@amaury1093 amaury1093 added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Dec 5, 2017
@jacogr jacogr added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Dec 6, 2017
@jacogr jacogr merged commit 7d0780d into master Dec 6, 2017
@jacogr jacogr deleted the am-visible-red branch December 6, 2017 09:44
@5chdn 5chdn added this to the 1.9 milestone Dec 6, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants