Skip to content
This repository has been archived by the owner on Apr 15, 2019. It is now read-only.

Bugfix: Registering delegate doesn't update UI - Closes #682 #688

Merged
merged 5 commits into from
Sep 1, 2017

Conversation

slaweet
Copy link
Contributor

@slaweet slaweet commented Aug 30, 2017

Closes #682

@slaweet slaweet self-assigned this Aug 30, 2017
@slaweet slaweet requested a review from reyraa August 30, 2017 13:06
@slaweet slaweet requested review from yasharAyari and removed request for reyraa August 31, 2017 08:59
Copy link
Contributor

@yasharAyari yasharAyari left a comment

Choose a reason for hiding this comment

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

we don't need delegateRegistration middleware. you can simply call getDelegate in account middleware.

@@ -6,7 +6,7 @@ const Address = (props) => {
const title = props.isDelegate ? 'Delegate' : 'Address';
const content = props.isDelegate ?
(<div>
<p className="inner primary">
<p className="inner primary delegate-name">
Copy link
Contributor

Choose a reason for hiding this comment

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

please remove delegate-name class and use an ID instead of that. when we want to select a an element in js , it is better to use ID.

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 added this class for e2e tests. All e2e tests use classes for selectors. It is not possible to change just this one. We can discuss if we should change classes to IDs in e2e tests, but it is definitely outside the scope of this PR.

@@ -0,0 +1,7 @@
export default {
Copy link
Contributor

Choose a reason for hiding this comment

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

we’re creating src/constants/transactionTypes.js when there are some other places to use this but you only uses this in one file.So, we don't need this constant.

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 use it in three files:

src/components/transactions/amount.js
src/store/middlewares/delegateRegistration.js
src/store/middlewares/delegateRegistration.test.js

@slaweet slaweet merged commit 27195e4 into development Sep 1, 2017
@slaweet slaweet deleted the 682-register-delegate-update branch September 1, 2017 07:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants