-
Notifications
You must be signed in to change notification settings - Fork 34
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
Add country form to React #1360
Conversation
Generated by 🚫 Danger |
There was a problem hiding this 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" |
There was a problem hiding this comment.
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: "" |
There was a problem hiding this comment.
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 => { | ||
|
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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}" */ /> |
There was a problem hiding this comment.
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:
mystamps/src/main/frontend/src/components/SimilarSeriesForm.js
Lines 105 to 109 in 778dc2a
<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> | ||
*/} |
There was a problem hiding this comment.
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:
mystamps/src/main/frontend/src/components/SimilarSeriesForm.js
Lines 111 to 113 in 778dc2a
<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) |
There was a problem hiding this comment.
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}" */ /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onChange={this.handleChange} /* th:field="*{nameRu}" */ /> | |
onChange={this.handleChange} /> |
value={this.state.nameInEnglish} | ||
name="nameInEnglish" | ||
required="required" | ||
onChange={this.handleChange} /* th:field="*{name}" */ /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
onChange={this.handleChange} /* th:field="*{name}" */ /> | |
onChange={this.handleChange} /> |
@splin-shady Hi, please, let me know if you need help or if you aren't going to work on this PR. Thanks! |
I'm closing this PR due to inactivity. Please, let me know if you want to continue this work. |
Address to #1345