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.

Sign up for GitHub

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

Animate transaction notifications & fix styles #305

Merged
merged 30 commits into from
Nov 7, 2017
Merged

Animate transaction notifications & fix styles #305

Show file tree
Hide file tree
Changes from 26 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
9ea3884
Add disclaimer modal to footer
Sep 29, 2017
6f2bd19
Merge branch 'develop' into update-disclaimer
james-prado Sep 29, 2017
60db6b4
Remove duplicate code & unnecessary styles
Oct 2, 2017
2aa4942
Fix formatting noise
Oct 3, 2017
88f6bd1
Merge branch 'update-disclaimer' of https://github.com/james-prado/My…
Oct 3, 2017
b9f5244
remove un-used css style
Oct 3, 2017
099cd0e
Fix tslint error & add media query for modals
Oct 5, 2017
46af5cf
Nest Media Query
Oct 5, 2017
244b4c0
Merge branch 'develop' into update-disclaimer
james-prado Oct 5, 2017
141ebec
Merge pull request #243 from james-prado/update-disclaimer
james-prado Oct 5, 2017
d991711
Merge branch 'develop' of https://github.com/MyEtherWallet/MyEtherWal…
Oct 5, 2017
038415d
Merge branch 'develop' of https://github.com/MyEtherWallet/MyEtherWal…
Oct 10, 2017
dfe3afe
Merge branch 'develop' of https://github.com/MyEtherWallet/MyEtherWal…
Oct 13, 2017
fb39318
Add react-transition-group
Oct 17, 2017
d189b49
Animate notifications with react-transition-group
Oct 17, 2017
ece40e8
Identify issue with notifications getting overridden
Oct 18, 2017
e8094af
Update RTG (react-transition-group) to v2 & identify keys as animatio…
Oct 19, 2017
e5fecd6
Add unique id on successful transactions for unique keys
Oct 19, 2017
9d3ab19
update classNames, remove unused import
Oct 19, 2017
d28da44
Merge branch 'develop' into css-issue
james-prado Oct 19, 2017
66b473e
Revert removing lodash
Oct 19, 2017
dbb5644
Remove unnecessary test util
Oct 24, 2017
fff9663
Remove formatting noise
Oct 25, 2017
fbfbc6c
Merge develop into css-issue
Oct 25, 2017
2c27004
Remove all formatting noise
Oct 25, 2017
176e5d5
Merge branch 'develop' into css-issue
Oct 25, 2017
cfee8d9
Update CSS & Change notification unique id
Oct 27, 2017
02f6fd4
Merge branch 'develop' into css-issue
Oct 27, 2017
7b7dfff
Add unique id for each notification
Oct 31, 2017
e8bd2bb
Merge branch 'develop' into css-issue
james-prado Oct 31, 2017
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
6 changes: 4 additions & 2 deletions common/actions/notifications/actionCreators.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,14 +6,16 @@ export type TShowNotification = typeof showNotification;
export function showNotification(
level: types.NOTIFICATION_LEVEL = 'info',
msg: ReactElement<any> | string,
duration?: number | types.INFINITY
duration?: number | types.INFINITY,
id?: number
): types.ShowNotificationAction {
return {
type: TypeKeys.SHOW_NOTIFICATION,
payload: {
level,
msg,
duration
duration,
id
}
};
}
Expand Down
1 change: 1 addition & 0 deletions common/actions/notifications/actionTypes.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ export type INFINITY = 'infinity';
export interface Notification {
level: NOTIFICATION_LEVEL;
msg: ReactElement<any> | string;
id?: number;
duration?: number | INFINITY;
}

Expand Down
20 changes: 20 additions & 0 deletions common/containers/TabSection/Notifications.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,3 +142,23 @@
}
}
}

.animation-enter {
opacity: 0.01;
}

.animation-enter.animation-enter-active {
opacity: 1;
transition: opacity 500ms;
}

.animation-exit {
opacity: 1;
transform: none;
}

.animation-exit.animation-exit-active {
opacity: 0.01;
transform: translateY(100%);
transition: opacity 500ms, transform 500ms;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Aaah, the ever familiar class naming of react-transition-group. Typically with these, I'm a big fan of:

.transitionname {
  &-enter {
    // ...
    &-active {
      // ...
    }
  }

  &-exit {
    // ...
    &-active {
      // ...
    }
  }
}

but this is completely stylistic

Copy link
Contributor Author

@james-prado james-prado Oct 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Yup, your right, it is stylistic but its important to maintain consistence. Still getting used to writing in suitcss. I'll make sure I update the css.

34 changes: 22 additions & 12 deletions common/containers/TabSection/Notifications.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,28 +5,38 @@ import {
} from 'actions/notifications';
import React from 'react';
import { connect } from 'react-redux';
import { TransitionGroup, CSSTransition } from 'react-transition-group';
import NotificationRow from './NotificationRow';
import './Notifications.scss';

interface Props {
notifications: Notification[];
closeNotification: TCloseNotification;
}

const Transition = props => (
<CSSTransition
{...props}
classNames="animation"
Copy link
Contributor

Choose a reason for hiding this comment

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

I would pick a more descriptive class than animation, this is likely to collide with some other usage of this elsewhere. Maybe Notification or something specific like that?

timeout={{ enter: 500, exit: 500 }}
/>
);

export class Notifications extends React.Component<Props, {}> {
public render() {
if (!this.props.notifications.length) {
return null;
}
return (
<div className="Notifications">
{this.props.notifications.map((n, i) => (
<NotificationRow
key={`${n.level}-${i}`}
notification={n}
onClose={this.props.closeNotification}
/>
))}
</div>
<TransitionGroup className="Notifications">
{this.props.notifications.map(n => {
return (
<Transition key={n.id}>
<NotificationRow
notification={n}
onClose={this.props.closeNotification}
/>
</Transition>
);
})}
</TransitionGroup>
Copy link
Contributor

Choose a reason for hiding this comment

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

I haven't looked at the latest react-transition-group in a while, but I'm pretty sure having both of these transition components is overkill. I think all you want is CSSTransition. Their example of CSSTransition doesn't use TransitionGroup at all.

Copy link
Contributor Author

@james-prado james-prado Oct 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

For animating individual elements, like in the CSSTransition example, TransitionGroup isn't necessary. But, if you want to manage multiple CSSTransition or Transition elements, TransitionGroup is required.

);
}
}
Expand Down
16 changes: 8 additions & 8 deletions common/containers/Tabs/SendTransaction/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -307,14 +307,14 @@ export class SendTransaction extends React.Component<Props, State> {
onChange={readOnly ? void 0 : this.onGasChange}
/>
{(offline || forceOffline) && (
<div>
<NonceField
value={nonce}
onChange={this.onNonceChange}
placeholder={'0'}
/>
</div>
)}
<div>
Copy link
Contributor

Choose a reason for hiding this comment

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

I see no changes here except for formatting.

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 just added a new commit to clean up the rest of the formatting noise, the one thing I couldn't change was the indentation you highlighted, it gets formatted that way by the precommit script in package.json.

<NonceField
value={nonce}
onChange={this.onNonceChange}
placeholder={'0'}
/>
</div>
)}
{unit === 'ether' && (
<DataField
value={data}
Expand Down
3 changes: 2 additions & 1 deletion common/sagas/wallet.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,7 +168,8 @@ function* broadcastTx(action: BroadcastTxRequestedAction): SagaIterator {
txHash={txHash}
blockExplorer={network.blockExplorer}
/>,
0
0,
txHash
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than refactor all notifications to have manually specified IDs, what if we generated random IDs for each of them? You could use Date.now() or Math.random()

Copy link
Contributor Author

@james-prado james-prado Oct 27, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Using Math.random() would be better for unique Id's like react keys, I used txHash because it was unique and already a property on the object.

)
);
yield put(broadcastTxSucceded(txHash, signedTx));
Expand Down
1 change: 0 additions & 1 deletion common/sass/styles/overrides/buttons.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -88,7 +88,6 @@
&:hover,
&:focus,
&.focus {
color: white;
text-decoration: none;
opacity: 1;
}
Expand Down
Loading