Post snippet #26
Post snippet #26
Conversation
.eslintrc.json
Outdated
| }], | ||
| "react/prop-types": [0] | ||
| "react/prop-types": [0], | ||
| "jsx-a11y/no-noninteractive-element-interactions": [0], |
There was a problem hiding this comment.
Alright, I do understand why jsx-a11y/click-events-have-key-events is needed. But why do we need this one? There's no rationale in the commit message.
There was a problem hiding this comment.
In our case we sometimes use click events on li, div, ul, etc elements which are not called non-interactive-elements. Since we can't move this events to other elements and we have this eslint rule enabled we will have an error. But with this rule disabled its possible for us to use such events
src/actions/index.js
Outdated
| export const postSnippet = snippet => dispatch => ( | ||
| fetch('http://api.xsnippet.org/snippets', { | ||
| method: 'POST', | ||
| mode: 'cors', |
There was a problem hiding this comment.
If I'm not mistaken, cors is default value. I'd suggest to do not mention this, as we don't set it explicitly for other requests we make.
| method: 'POST', | ||
| mode: 'cors', | ||
| headers: { | ||
| Accept: 'application/json', |
There was a problem hiding this comment.
Can we use quotes here? Just to be consistent with 'Content-Type'.
There was a problem hiding this comment.
Our lint will throw an error if I will use quotes here
There was a problem hiding this comment.
we can add rule to always use quotes for values if this will help feel better
src/components/NewSnippet.jsx
Outdated
|
|
||
| onInputChange(e) { | ||
| const { name, value } = e.target; | ||
| const result = (name === 'tags') |
There was a problem hiding this comment.
Geez.. I'm not insist, but using plain if would be more readable (and readability matters!).
onInputChange(e) {
const { name, value } = e.target;
if (name === 'tags') {
value = value.split(',').map(item => item.trim())
}
this.setState({ [name]: value })
}There was a problem hiding this comment.
will change, but not exactly like you proposed, since value variable already defined this will occur an error
src/components/NewSnippet.jsx
Outdated
| ? value.split(',').map(item => item.trim()) | ||
| : value; | ||
|
|
||
| this.setState({ [name]: result }); |
There was a problem hiding this comment.
Why name is wrapped in brackets (i.e. [name])?
There was a problem hiding this comment.
I want to reuse this function for a few similar inputs, and I want to set their values in state, so I will have: {tags: some_value} and {title: some_other_value}.
If I will use name without [] it will think that I want this {name: some_value}.
In this case js syntax lets me to use name as different key
There was a problem hiding this comment.
Oh, gosh, Jesus.. so basically if you use name without brackets, it will think that name is a name of key?
| } | ||
|
|
||
| postSnippet(e) { | ||
| e.preventDefault(); |
There was a problem hiding this comment.
I'd add few lines with explanations why preventDefault() is necessary. If I'm not mistaken it's because we don't want to follow default HTML behavior when form data is submitted to specified URI using POST and multipart/form-data content type.
There was a problem hiding this comment.
hm.It seems obvious to me
| } | ||
|
|
||
| onSyntaxClick(syntax) { | ||
| this.setState({ syntax }); // eslint-disable-line react/no-unused-state |
There was a problem hiding this comment.
Why do we need this ignore rule in two places?
There was a problem hiding this comment.
because our lint checker creates to error messages for those two lines
src/components/Syntaxes.jsx
Outdated
| onClickHandler(e) { | ||
| const { index, syntax } = e.target.dataset; | ||
|
|
||
| this.setState({ activeItem: { [index]: true } }); |
There was a problem hiding this comment.
I think I have few questions here:
- Why do we need to wrap
indexin brackets (i.e.[index])? What does it mean? Why just notindex? - Why does
activeItemis an object with just oneboolwhich can't be evenfalse? I'd suggest to use the following schema:What do you think?this.setState({ activeIndex: index })
- It seems like
indexis not read from state, so why define one?
There was a problem hiding this comment.
- look on one of answers above about these [] brackets;
- and 3. Take a look on line 39 where I check is newly selected item has appropriate index, in this way I can avoid additional loops that will do nothing more than remove active class from previous item
lotrien
force-pushed
the
lang-bar
branch
3 times, most recently
from
544537b to
9a8e3cb
Compare
since we find this not actual for our project, due to appropriate work onClick events on different keypress events.
In scope of this pr basic functionality for snippet posting were implemented.
In scope of this pr basic functionality for snippet posting were implemented.