Skip to content
This repository has been archived by the owner on Jul 9, 2021. It is now read-only.

[instant] Asset buyer errors #1142

Merged
merged 18 commits into from
Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions packages/instant/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,7 @@
"react-dom": "^16.5.2",
"react-redux": "^5.0.7",
"redux": "^4.0.0",
"redux-devtools-extension": "^2.13.5",
"styled-components": "^3.4.9",
"ts-optchain": "^0.1.1"
},
Expand Down
54 changes: 54 additions & 0 deletions packages/instant/src/components/animations/slide_animations.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,54 @@
import * as React from 'react';

import { keyframes, styled } from '../../style/theme';

const slideKeyframeGenerator = (fromY: string, toY: string) => keyframes`
from {
position: relative;
top: ${fromY};
}

to {
position: relative;
top: ${toY};
}
`;

export interface SlideAnimationProps {
keyframes: string;
animationType: string;
animationDirection?: string;
}

export const SlideAnimation =
styled.div <
SlideAnimationProps >
`
animation-name: ${props => props.keyframes};
animation-duration: 0.3s;
animation-timing-function: ${props => props.animationType};
animation-delay: 0s;
animation-iteration-count: 1;
animation-fill-mode: ${props => props.animationDirection || 'none'};
position: relative;
`;

export interface SlideAnimationComponentProps {
downY: string;
}

export const SlideUpAnimation: React.StatelessComponent<SlideAnimationComponentProps> = props => (
<SlideAnimation animationType="ease-in" keyframes={slideKeyframeGenerator(props.downY, '0px')}>
{props.children}
</SlideAnimation>
);

export const SlideDownAnimation: React.StatelessComponent<SlideAnimationComponentProps> = props => (
<SlideAnimation
animationDirection="forwards"
animationType="cubic-bezier(0.25, 0.1, 0.25, 1)"
keyframes={slideKeyframeGenerator('0px', props.downY)}
>
{props.children}
</SlideAnimation>
);

This file was deleted.

20 changes: 3 additions & 17 deletions packages/instant/src/components/asset_amount_input.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
import { AssetProxyId } from '@0xproject/types';
import { BigNumber } from '@0xproject/utils';
import * as _ from 'lodash';
import * as React from 'react';

import { assetMetaData } from '../data/asset_meta_data';
import { assetDataUtil } from '../util/asset_data';

import { ColorOption } from '../style/theme';
import { util } from '../util/util';

Expand All @@ -26,26 +26,12 @@ export class AssetAmountInput extends React.Component<AssetAmountInputProps> {
<AmountInput {...rest} onChange={this._handleChange} />
<Container display="inline-block" marginLeft="10px">
<Text fontSize={rest.fontSize} fontColor={ColorOption.white} textTransform="uppercase">
{this._getAssetSymbolLabel()}
{assetDataUtil.bestNameForAsset(this.props.assetData, '???')}
</Text>
</Container>
</Container>
);
}
private readonly _getAssetSymbolLabel = (): string => {
const unknownLabel = '???';
if (_.isUndefined(this.props.assetData)) {
return unknownLabel;
}
const metaData = assetMetaData[this.props.assetData];
if (_.isUndefined(metaData)) {
return unknownLabel;
}
if (metaData.assetProxyId === AssetProxyId.ERC20) {
return metaData.symbol;
}
return unknownLabel;
};
private readonly _handleChange = (value?: BigNumber): void => {
this.props.onChange(value, this.props.assetData);
};
Expand Down
38 changes: 32 additions & 6 deletions packages/instant/src/components/instant_heading.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import * as React from 'react';

import { SelectedAssetAmountInput } from '../containers/selected_asset_amount_input';
import { ColorOption } from '../style/theme';
import { AsyncProcessState } from '../types';
import { format } from '../util/format';

import { Container, Flex, Text } from './ui';
Expand All @@ -12,20 +13,45 @@ export interface InstantHeadingProps {
selectedAssetAmount?: BigNumber;
totalEthBaseAmount?: BigNumber;
ethUsdPrice?: BigNumber;
quoteState: AsyncProcessState;
Copy link
Contributor

Choose a reason for hiding this comment

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

I still think this should be called quoteRequestState. quote is data, and it doesn't make sense to me for it to have state.

}

const displaytotalEthBaseAmount = ({ selectedAssetAmount, totalEthBaseAmount }: InstantHeadingProps): string => {
const Placeholder = () => (
<Text fontWeight="bold" fontColor={ColorOption.white}>
&mdash;
</Text>
);
const displaytotalEthBaseAmount = ({
selectedAssetAmount,
totalEthBaseAmount,
}: InstantHeadingProps): React.ReactNode => {
if (_.isUndefined(selectedAssetAmount)) {
return '0 ETH';
}
return format.ethBaseAmount(totalEthBaseAmount, 4, '...loading');
return format.ethBaseAmount(totalEthBaseAmount, 4, <Placeholder />);
};

const displayUsdAmount = ({ totalEthBaseAmount, selectedAssetAmount, ethUsdPrice }: InstantHeadingProps): string => {
const displayUsdAmount = ({
totalEthBaseAmount,
selectedAssetAmount,
ethUsdPrice,
}: InstantHeadingProps): React.ReactNode => {
if (_.isUndefined(selectedAssetAmount)) {
return '$0.00';
}
return format.ethBaseAmountInUsd(totalEthBaseAmount, ethUsdPrice, 2, '...loading');
return format.ethBaseAmountInUsd(totalEthBaseAmount, ethUsdPrice, 2, <Placeholder />);
};

const loadingOrAmount = (quoteState: AsyncProcessState, amount: React.ReactNode): React.ReactNode => {
if (quoteState === AsyncProcessState.PENDING) {
return (
<Text fontWeight="bold" fontColor={ColorOption.white}>
&hellip;loading
</Text>
);
} else {
return amount;
}
};

export const InstantHeading: React.StatelessComponent<InstantHeadingProps> = props => (
Expand All @@ -47,11 +73,11 @@ export const InstantHeading: React.StatelessComponent<InstantHeadingProps> = pro
<Flex direction="column" justify="space-between">
<Container marginBottom="5px">
<Text fontSize="16px" fontColor={ColorOption.white} fontWeight={500}>
{displaytotalEthBaseAmount(props)}
{loadingOrAmount(props.quoteState, displaytotalEthBaseAmount(props))}
Copy link
Contributor

Choose a reason for hiding this comment

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

Feels kind of weird to have two nested Text components. Can't displayTotalEthUsdAmount take care of all of this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call -- yes, will fix

</Text>
</Container>
<Text fontSize="16px" fontColor={ColorOption.white} opacity={0.7}>
{displayUsdAmount(props)}
{loadingOrAmount(props.quoteState, displayUsdAmount(props))}
</Text>
</Flex>
</Flex>
Expand Down
24 changes: 16 additions & 8 deletions packages/instant/src/components/sliding_error.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ import * as React from 'react';

import { ColorOption } from '../style/theme';

import { SlideUpAndDownAnimation } from './animations/slide_up_and_down_animation';
import { SlideDownAnimation, SlideUpAnimation } from './animations/slide_animations';

import { Container, Text } from './ui';

Expand All @@ -20,17 +20,25 @@ export const Error: React.StatelessComponent<ErrorProps> = props => (
borderRadius="6px"
marginBottom="10px"
>
<Container marginRight="5px" display="inline">
{props.icon}
<Container marginRight="5px" display="inline" top="3px" position="relative">
<Text fontSize="20px">{props.icon}</Text>
</Container>
<Text fontWeight="500" fontColor={ColorOption.darkOrange}>
{props.message}
</Text>
</Container>
);

export const SlidingError: React.StatelessComponent<ErrorProps> = props => (
<SlideUpAndDownAnimation downY="120px" delayMs={5000}>
<Error icon={props.icon} message={props.message} />
</SlideUpAndDownAnimation>
);
export type SlidingDirection = 'up' | 'down';
export interface SlidingErrorProps extends ErrorProps {
direction: SlidingDirection;
}
export const SlidingError: React.StatelessComponent<SlidingErrorProps> = props => {
const AnimationComponent = props.direction === 'up' ? SlideUpAnimation : SlideDownAnimation;

return (
<AnimationComponent downY="120px">
<Error icon={props.icon} message={props.message} />
</AnimationComponent>
);
};
4 changes: 4 additions & 0 deletions packages/instant/src/components/zero_ex_instant_container.tsx
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import * as React from 'react';

import { LatestBuyQuoteOrderDetails } from '../containers/latest_buy_quote_order_details';
import { LatestError } from '../containers/latest_error';
import { SelectedAssetBuyButton } from '../containers/selected_asset_buy_button';
import { SelectedAssetInstantHeading } from '../containers/selected_asset_instant_heading';

Expand All @@ -12,6 +13,9 @@ export interface ZeroExInstantContainerProps {}

export const ZeroExInstantContainer: React.StatelessComponent<ZeroExInstantContainerProps> = props => (
<Container width="350px">
<Container zIndex={1} position="relative">
<LatestError />
</Container>
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥

<Container
zIndex={2}
position="relative"
Expand Down
35 changes: 35 additions & 0 deletions packages/instant/src/containers/latest_error.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
import * as React from 'react';

import { connect } from 'react-redux';

import { SlidingError } from '../components/sliding_error';
import { LatestErrorDisplay, State } from '../redux/reducer';
import { errorUtil } from '../util/error';

export interface LatestErrorComponentProps {
assetData?: string;
latestError?: any;
Copy link
Contributor Author

@steveklebanoff steveklebanoff Oct 17, 2018

Choose a reason for hiding this comment

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

Per convo w/ @fragosti , we explored two options:

  1. When error is "cleared" (after timeout), set currentError to be undefined. This would have been nice because it's the most straightforward representation of the current state, but animating the un-mounting of the component would either be pretty hairy or involve introducing a new dependency.

  2. Keep the error around in latestError but set latestErrorDismissed to true, and then render the sliding down error message. I don't love keeping the last error around but treating it as "dismissed", but it makes the animation logic simple.

After discussing, I went with option 2 for the drawbacks listed in option 1. Open to feedback though.

slidingDirection: 'down' | 'up';
}

export const LatestErrorComponent: React.StatelessComponent<LatestErrorComponentProps> = props => {
if (!props.latestError) {
return <div />;
}
const { icon, message } = errorUtil.errorDescription(props.latestError, props.assetData);
return <SlidingError direction={props.slidingDirection} icon={icon} message={message} />;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

A convention I'm trying to follow is to not have any Component code in these container files, they simply connect a pure presentational component to the redux store and do any state -> prop mapping. Seems like you don't need this component to me?

  • The sliding direction logjc can just be done in mapStateToProps.
  • icon is a pure function of state.
  • message is a pure function of state.

The only thing is not rendering when latestError is not defined, but do we need that? The error hides behind container.

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 like the idea of getting rid of this component, but I think we need the custom "not render when latestError is undefined" logic. Some thoughts:

  • If there is no latestError, we wouldn't have anything to send in to SlidingError's icon and message props. I like keeping these props as required instead of optional.
  • Even if we rendered SlidingError without icon and message props, in the current implementation, rendering it in the "Slide Down" phase would have it pop up at the top and slide down, which is not what we want. We could modify SlidingError to return a blank div if icon and message are empty, but that doesn't feel quite right to me.

I may be missing something though, let me know if there is an alternative approach in which we could get rid of this component.

If you agree we need to keep the component, I understand the convention and pattern of separating presentational and container components, but in this case it seems overboard IMHO: this component is extremely simple and it's highly unlikely it'd be used anywhere else.

Copy link
Contributor

Choose a reason for hiding this comment

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

You could change it so that latestError sticks around and you simply toggle it up and down in the flasher / redux. This kind of seems better to me because then it becomes impossible to make the error just disappear?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Per slack convo, decided to leave as-is and address later if needed


interface ConnectedState {
assetData?: string;
latestError?: any;
slidingDirection: 'down' | 'up';
}
export interface LatestErrorProps {}
const mapStateToProps = (state: State, _ownProps: LatestErrorProps): ConnectedState => ({
assetData: state.selectedAssetData,
latestError: state.latestError,
slidingDirection: state.latestErrorDisplay === LatestErrorDisplay.Present ? 'up' : 'down',
});

export const LatestError = connect(mapStateToProps)(LatestErrorComponent);
Loading