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.

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

(Refactor) Use final form step three #767

Merged
merged 52 commits into from
Apr 9, 2018
Merged

Conversation

fernandomg
Copy link
Contributor

@fernandomg fernandomg commented Apr 4, 2018

This is another step towards the completion of #711

In this PR StepTree was moved entirely into final-form, with the exception of whitelist which is now part of stepThreeForm (similar to what was done with reservedTokens in stepTwo).

I didn't manage to prevent error messages for rate and supply for the Tiers added after the first one. This won't affect the UX, but will display the errors by default.

Besides I've created an Error component, which makes easier to display errors related to final-form Fields.

This looks still as a WIP, but the idea is to keep moving toward the adoption of final-form as fast as possible.

Franco Victorio and others added 30 commits March 27, 2018 16:37
…use-final-form-step-three

# Conflicts:
#	src/components/stepThree/index.js
@fernandomg
Copy link
Contributor Author

@dennis00010011b this PR sligtly changed the DOM structure of the Step Three, and is giving errors with e2e tests.

@dennis00010011b
Copy link

@fernandomg I've updated e2e script, test for upload CSV should work now.
Try to update submodule token-wizard-test-automation and commit again

@ghost ghost assigned fvictorio Apr 5, 2018
/>
) : null}
<p className="description">Slow is cheap, fast is expensive</p>
<Error name="gasPrice"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

The name shouldn't be hardcoded here, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

}

export const Error = ({ name, errorStyle }) => (
<Field
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure I understand this. Is the Field component really necessary? Couldn't we just receive the whole meta object from props and use that?

If I'm understanding this correctly, this means that we'll have two Fields with the same name attribute, but one of them is used only to show errors. It doesn't sound right to me, is there a good reason for this approach?

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 a way to make the Error component react only to a subset of items' changes (subscription) for the specified field.

* But it worked for me to keep the error messages properly updated for the minCap field.
*/
}
<Field name="minCap" subscription={{}}>
Copy link
Contributor

Choose a reason for hiding this comment

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

Same here thing (as in the Error component). Is the Field necessary? Can't we use the change function?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change function is a Form's property, which will force us to transfer props from StepThreeForm to TierBlock.

Whereas keeping the current approach makes easier for us to extend this sole component, without needing to modify any parent component.

@fernandomg
Copy link
Contributor Author

@dennis00010011b I've been running the e2e locally and it is still failing.

This are the lists of issues that I found:

  1. For the error messages you use the xpath selector and look for the only attr you had available that is style, I'd slightly changed the styles for the stepThree and now it's failing. As a result I've just added the class="error" to the errors <p> tags.
  2. Wizard step#3: User is able to bulk delete all whitelisted addresses is failing for me, despite I reproduced the steps in the browser and worked properly.
  3. All test that contains the assert assert.equal(flagCrowdsale, true); fail in the testSuite1.js (you can see them here: https://travis-ci.org/poanetwork/token-wizard/builds/362808731#L2724)

As a note, with this migration to final-form we're adding name attributes to most of the inputs in the wizard, so it will be easier and cleaner for you to look for a particular tag despite its location in the DOM.

If you find yourself using XPath to reach a particular element, don't hesitate in request a change in the code to add an id, a name or a class, or whatever attribute is needed, to make it easier for you to build the tests.

@dennis00010011b
Copy link

@fernandomg Thank you for feedback.

  1. I’ll change finding the errors by class name. It would really helpful if elements would have id or at least name or class.
  2. I see this issue with bulk delete, still can’t figure out it
  3. Almost all test use pre-created crowdsale . flagCrowdsale used for skip tests if crowdsale hasn’t created.

@vbaranov vbaranov merged commit 1c41784 into master Apr 9, 2018
@ghost ghost removed the awaiting for review label Apr 9, 2018
@vbaranov vbaranov deleted the use-final-form-step-three branch April 10, 2018 10:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants