Skip to content

Conversation

@AlexKVal
Copy link
Contributor

@AlexKVal AlexKVal commented Aug 13, 2016

Partially resolves #295

Simple React page.

I probably will try to fix other pages as well.
(Later. When I feel myself less sick.)


This change is Reviewable

@justin808
Copy link
Member

@AlexKVal Looks outstanding.


Reviewed 2 of 2 files at r1.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx, line 19 [r1] (raw file):

  if (error) {
    const errorData = (error && error.response && error.response.data) || {};
    return (propName in errorData) ? 'error' : 'success';

I've hardly ever used the in operator in JS. @alexfedoseev should we be using it more? I usually use _.has: https://lodash.com/docs#has


client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx, line 132 [r1] (raw file):

            disabled={this.props.isSaving}
            hasFeedback={true}
            bsStyle={bsStyleFor('author', this.props.error)}

Nice to be adding this!


client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx, line 145 [r1] (raw file):

            disabled={this.props.isSaving}
            hasFeedback={true}
            bsStyle={bsStyleFor('text', this.props.error)}

react-bootstrap/react-bootstrap#1765

react-bootstrap/react-bootstrap#1826

Should we change Input to something else?


client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx, line 254 [r1] (raw file):

    const errorElements = _.transform(errorData, (result, errorText, errorFor) => {
      result.push(<li key={errorFor}><b>{_.upperFirst(errorFor)}:</b> {errorText}</li>);
    }, []);

Nice


Comments from Reviewable

@AlexKVal
Copy link
Contributor Author

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx, line 19 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

I've hardly ever used the in operator in JS. @alexfedoseev should we be using it more? I usually use _.has: https://lodash.com/docs#has

My motivations: The project already uses ES6 transpiler hence it is OK to use "in".

"_.has" is one more dependency.

I rewrite it if it is more preferable.


client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx, line 145 [r1] (raw file):

Previously, justin808 (Justin Gordon) wrote…

react-bootstrap/react-bootstrap#1765

react-bootstrap/react-bootstrap#1826

Should we change Input to something else?

I wanted to postpone RB dependency updating to another PR and decided to solve the task at hand with as minimum side-effects as it is possible 😄

Comments from Reviewable

@alex35mil
Copy link
Member

Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx, line 19 [r1] (raw file):

Previously, AlexKVal (Alexander Shemetovsky) wrote…

My motivations:
The project already uses ES6 transpiler hence it is OK to use "in".

"_.has" is one more dependency.

I rewrite it if it is more preferable.

Between native and lib ways I always choose native, so my opinion: as `in` operator is standard, it absolutely legit to use it here.

Comments from Reviewable

@AlexKVal
Copy link
Contributor Author

AlexKVal commented Aug 15, 2016

I've added the fix for the "React Demo" page.
Now it works too.

Now I will rebase the branch to master to resolve current conflicts.

@AlexKVal AlexKVal force-pushed the show-errors branch 2 times, most recently from 0fa88df to 2704719 Compare August 15, 2016 16:24
And resolve eslint warnings because of Eslint (shakacode#320)
@AlexKVal
Copy link
Contributor Author

I've rebased the PR onto the latest master.

Should I update React-Bootstrap dependency (and with it all the form elements) within this PR
or new one ?
If in the new one, then I'll do it after this PR has merged.

🍒

@justin808
Copy link
Member

@AlexKVal The key thing we need is a capybara test that verifies this across the page. If we had that test, we would have caught this issue long ago.

Otherwise, looking fabulous!


Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions.


client/app/bundles/comments/components/CommentBox/CommentForm/CommentForm.jsx, line 145 [r1] (raw file):

Previously, AlexKVal (Alexander Shemetovsky) wrote…

I wanted to postpone RB dependency updating to another PR and decided to solve the task at hand with as minimum side-effects as it is possible 😄

@AlexKVal, @dylangrafmyre and @robwise would appreciate this philosophy of keeping PRs focused! Thank you for this insight!

Comments from Reviewable

@AlexKVal
Copy link
Contributor Author

@AlexKVal The key thing we need is a capybara test

Then I'll learn Capybara and add tests with it.

@AlexKVal
Copy link
Contributor Author

I added tests.
They not only check for validation errors displaying,
but what should happen after a consequent successful submitting as well.

@justin808
Copy link
Member

:lgtm_strong:

Fabulous!


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, 4 unresolved discussions, some commit checks failed.


Comments from Reviewable

@justin808 justin808 merged commit be20d08 into shakacode:master Aug 21, 2016
@AlexKVal AlexKVal deleted the show-errors branch August 21, 2016 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Demo App Server Error

3 participants