Cleanup #5
Cleanup #5
Conversation
| export default function gittenify(store) { | ||
| const userMap = {}; | ||
|
|
||
| for (const u of store.users) { |
There was a problem hiding this comment.
const u seems strange here. Wouldn't let fit better? Semantically.
There was a problem hiding this comment.
@RobinThrift const should always be used when you know that the variable will not be modified. Actually the eslint config I added errors here when you use let because u never changes.
There was a problem hiding this comment.
That's what I meant by "Semantically". While the variable inside the block itself doesn't change, the variable "itself" does. If it checks out, it's fine. Just a bit odd ;)
There was a problem hiding this comment.
hmm interesting, I guess since you are re-declaring it with a const keyword on every turn of that for-loop it is like you are just wiping it away and then initializing it fresh every time so that the variable is actually not being changed but erased and re-declared.
There was a problem hiding this comment.
or something...
might need some further research.
There was a problem hiding this comment.
Babel doesn't seem to have a problem with it. Good catch though!
There was a problem hiding this comment.
Babel transforms this into for(var u,l=e.users[Symbol.iterator]();!(a=(u=l.next()).done);a=!0). I think the variables in the for-head only get initialized once, hence why I caught it. I could be wrong though. ES6 sure is interesting ;)
There was a problem hiding this comment.
Interesting. Then the linter is not working correctly in this case I would say.
There was a problem hiding this comment.
I changed it to let and turned off the prefer-const eslint rule :)
| export default function replaceUsers(el, store, userMap) { | ||
| const containers = el.querySelectorAll(containerSelector); | ||
|
|
||
| [...containers].forEach((container) => { |
There was a problem hiding this comment.
Great use of the spread operator. I love this ES6 stuff 😄
There was a problem hiding this comment.
yeah so neat!!!
that's actually es7 as far as I know :))
| import replaceUsers from './replaceUsers'; | ||
|
|
||
| export default function gittenify(store) { | ||
| const userMap = {}; |
There was a problem hiding this comment.
We could use a "real" ES6 Map here, just for ES6's sake. But I don't know, how that would play with Chrome's storage. Maybe like this: http://www.2ality.com/2015/08/es6-map-json.html
There was a problem hiding this comment.
good idea, but that is out of scope of this pull request. I just want to clean up and give it some structure. You are very welcome to create a new pull request with that feature added though 😄
There was a problem hiding this comment.
Just wanted to mention it for "future reference" ;) Maybe some day I will have time...
There was a problem hiding this comment.
Just open an issue for that and label it with improvement or something like that :)
|
👍 Okay, looks good to me. Somebody else might want to check if it works in their browser too. 🐱 |
|
@RobinThrift thank you! |
|
That's good enough for me right now. I am merging this in :) |
|
Okay, great! Cool stuffs! |
Introduced some config for linting, editorconfig, gitignore etc.
Also refactored towards es2015 and modularized a bit.