Skip to content

Add country form to React #1360

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

Conversation

splin-shady
Copy link

Address to #1345

@splin-shady splin-shady requested a review from php-coder as a code owner April 21, 2020 13:58
@mystamps-bot
Copy link

16 Errors
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/logic.robot#L12-L19:
Test case Create country with name in English (fill only mandatory fields) fails with message:
Setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/logic.robot#L21-L28:
Test case Create country with name in English and Russian fails with message:
Setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/misc.robot#L10-L15:
Test case Country name should be stripped from leading and trailing spaces fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/misc.robot#L17-L22:
Test case Country name should be modified by replacing multiple spaces by one fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/misc.robot#L24-L29:
Test case Country name in English should accept all allowed characters fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/misc.robot#L31-L36:
Test case Country name in Russian should accept all allowed characters fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L10-L15:
Test case Create country with too short name fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L17-L24:
Test case Create country with too long name fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L26-L31:
Test case Create country with forbidden characters in name fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L33-L38:
Test case Create country with repeating hyphens in name fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L40-L45:
Test case Create country with name that starts with hyphen fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L47-L52:
Test case Create country with name that ends with hyphen fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L54-L59:
Test case Create country with existing (non-unique) name fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L61-L66:
Test case Create country with existing name but in a different case fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L68-L73:
Test case Create country with non-existing name but existing (non-unique) slug fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
🚫 robotframework-maven-plugin error in src/test/robotframework/country/creation/validation.robot#L75-L85:
Test case Create country with forbidden names fails with message:
Parent suite setup failed:
com.gargoylesoftware.htmlunit.ScriptException: ReferenceError: “AddCountryForm” is not defined. (script in http://127.0.0.1:8080/country/add from (78, 11) to (80, 12)#79)
1 Warning
⚠️ danger check: pull request description doesn’t contain a link to original issue.
Consider adding a comment in the following format: Addressed to #XXX where XXX is an issue number
1 Message
📖 robotframework-maven-plugin reported about 16 errors. Please, fix them. See also: https://github.com/php-coder/mystamps/wiki/integration-tests

Generated by 🚫 Danger

@php-coder php-coder mentioned this pull request Apr 21, 2020
Copy link
Owner

@php-coder php-coder left a comment

Choose a reason for hiding this comment

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

Thank you for the contribution! 👍

I left some comments, but overall the following is missing at this moment:

  • logic for sending a request to a server should be added
  • all phrases should be localized
  • field errors should be shown
  • form errors should be shown

@@ -73,43 +73,8 @@ <h3 th:text="${#strings.capitalize(add_country)}">
</small>
</div>

<form id="add-country-form" method="post" class="form-horizontal" action="info.html"
Copy link
Owner

Choose a reason for hiding this comment

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

We shouldn't remove the form in this PR. Everything should continue to work until we fully re-implement existing logic.

Can we try to render the component into id="add-country-form" or we need a separate element for that?

super(props);
this.state = {
nameInEnglish: "",
nameInRussian: ""
Copy link
Owner

Choose a reason for hiding this comment

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

Please, prefer single quotes over double one.

axios.post(
"categories",{},{}
).then(response => {

Copy link
Owner

Choose a reason for hiding this comment

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

Do you plan to implement this logic?

type="text"
className="form-control"
value={this.state.nameInEnglish}
name="nameInEnglish"
Copy link
Owner

Choose a reason for hiding this comment

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

Is this "name" attribute is needed for event.target.name?

@@ -0,0 +1,89 @@
class AddCountryForm extends React.Component {
Copy link
Owner

Choose a reason for hiding this comment

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

Could you prepend a comment like this?

//
// IMPORTANT:
// You must update ResourceUrl.RESOURCES_VERSION each time whenever you're modified this file!
//

<div className="col-sm-offset-4 col-sm-5">
<input type="submit"
className="btn btn-primary"
value="Add" /* th:value="#{t_add}" */ />
Copy link
Owner

Choose a reason for hiding this comment

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

We should add localization and convert it into a button. Like there:

<button type="submit"
className="btn btn-primary btn-sm"
disabled={this.state.isDisabled}>
{ this.props.l10n['t_mark_as_similar'] || 'Mark as similar' }
</button>

onChange={this.handleChange} /* th:field="*{nameRu}" */ />
{/*
<span id="nameRu.errors" class="help-block" th:if="${#fields.hasErrors('nameRu')}" th:each="error : ${#fields.errors('nameRu')}" th:text="${error}"></span>
*/}
Copy link
Owner

Choose a reason for hiding this comment

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

Should be implemented like there:

<span id="similar-id.errors" className={`help-block ${hasValidationErrors ? '' : 'hidden'}`}>
{ this.state.validationErrors.join(', ') }
</span>


<div className="form-group" /* th:classappend="${#fields.hasErrors('nameRu') ? 'has-error' : ''}" */>
<label htmlFor="nameRu" className="control-label col-sm-4" /* th:text="#{t_name_in_russian}" */>
Name (in Russian)
Copy link
Owner

Choose a reason for hiding this comment

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

Localization should be supported.

className="form-control"
value={this.state.nameInRussian}
name="nameInRussian"
onChange={this.handleChange} /* th:field="*{nameRu}" */ />
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
onChange={this.handleChange} /* th:field="*{nameRu}" */ />
onChange={this.handleChange} />

value={this.state.nameInEnglish}
name="nameInEnglish"
required="required"
onChange={this.handleChange} /* th:field="*{name}" */ />
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
onChange={this.handleChange} /* th:field="*{name}" */ />
onChange={this.handleChange} />

@php-coder
Copy link
Owner

@splin-shady Hi, please, let me know if you need help or if you aren't going to work on this PR. Thanks!

@php-coder
Copy link
Owner

I'm closing this PR due to inactivity. Please, let me know if you want to continue this work.

@php-coder php-coder closed this May 18, 2020
@php-coder php-coder added the resolution/gone Non-reproducible, obsolete, outdated issues label May 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
resolution/gone Non-reproducible, obsolete, outdated issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants