Skip to content
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

はじめてのぷるりく #2

Open
wants to merge 27 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@nezumi650
Copy link
Owner

commented Apr 11, 2013

れびゅーをお願いします


// ボタン画像の背景色
// @TODO もっとかっこよくエフェクトさせる
var buttonSettingColor = 'white';

This comment has been minimized.

Copy link
@gilbite

gilbite Apr 11, 2013

このスペースのずれはなんなの

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

ダメダメですね。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

正直すみませんでした

@@ -0,0 +1,288 @@

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

とりあえず "use strict"; してlintしてからprやりなおしな。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

use strict知らなかった!!!!
やってみます!!!!

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

use strictされてるコードなんて某社内ではみたことないです!

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

100理ぐらいあって1害もないからやっとけ!

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

そすね!ちなみに面接でuse strictしてあるコードを見せてもらったことがあって、「こいつできる!」ってなりましたw

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

JSLintが厳しすぎたらJSHintでもいいのよ。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 13, 2013

Author Owner

"use strict"; 追加してみたけど何も起こらないので、使い方調べ中。。。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 14, 2013

Author Owner

"use strict"; 追加したけど、べつに何も起こらない。
なぜなんだ。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 14, 2013

Author Owner

firefox で検証したら、めっちゃ色々出てました。
色々直します。


/**
* 【!!!】この設定値は変更しないでください。汎用性が著しく失われます。
*/

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

定数名になんか規約ないの、よみずらい。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 13, 2013

Author Owner

なおした

function addIINEButtonComments() {
var commentElms = document.querySelectorAll( '[id^="issuecomment-"]' );

for (var i = 0; i < commentElms.length; i++) {

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

どの言語でもそうだけど ループの条件式でlengthとったらあかん。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

ループの外でとって、変数に入れて、ループ内ではそれを利用するべきって事ですか?
確かにそうだ。だめだ。

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

イディオム的には
var i = 0 , n = xxx.length ; i < n ; i ++ で覚えとけばいい。

文脈に応じて適当にアレンジ。

This comment has been minimized.

Copy link
@haruton

haruton Apr 11, 2013

これってPHPでいうと
for ( $i = 0; $i < count($array); $i++ )
ってことなのかしら?

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

そこまでシビアに考えなくてもいいんじゃない派です。
見やすいのは i < array.lengthの方かなとも思いますし。
多分for文の中で回している処理(DOM操作)のほうが重いでしょうし。

あと、arrayが動的に変化するもの(document.getElementByTagsなどで取ってきた配列)だと罠にはまったりするかも。

こんなコード書いてみましたけど、3msしか違いがなかったです。
ちなみにvar len = array.length;したほうが遅かったです(MacのChromeのコンソール)

var array = [];
for (var i = 0; i < 100000; i++) { array.push(i); }

/* 160ms
console.time('a');
var len = array.length;
for (var i = 0; i < len; i++){ array[i]; }
console.timeEnd('a');
*/

/* 157ms
console.time('a');
for (var i = 0; i < array.length; i++){ array[i]; }
console.timeEnd('a');
*/

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

IEが速くなったらおれもこんなこと言わない。
いやーレビューしてる側の意見が対立してからが本番ですよね。面白くなってきた。

This comment has been minimized.

Copy link
@h13i32maru

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

あとJS初心者へのレビューってこともあって安定パターンになるようにレビューしてるっていうのもある。
例えばどんなPHPの習作でもCSRFとかXSSとかあったら突っ込むでしょ?そういう感じです。

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

IEの下り、みてなかった。
IE対応とか人生で一度もしたことのない幸せものなので、完全にスルーしてました。

あと、CSRFの件も確かにそのとおりすね〜。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

勉強になりました!
直しません!

*/
function getAllIINEComments() {
var discussionBubbles = document.querySelectorAll('.discussion-bubble');
var IINEComments = {}; //空のオブジェクト

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

upperとlowerが混じっててよみずらい。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

'IINE' に、固有名詞的な意味を持たせたかったんですが、
やっぱり読みづらいすかねぇ。。素直に Iine にするか

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

Iine完全にline(ライン)に空目する

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

たぶん like なんじゃないかな・・・。

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

//空のオブジェクトって{}が空であることは自明なんだけど、何かコメントに別の意味があるのかな〜。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

そうか、私以外に不要なコメントなんだな、、、 > // 空のオブジェクト

* いいねコメントを非表示にする
*/
function hideAllIINEComments(discussionBubblesAllArray) {
for (var parentId in discussionBubblesAllArray){

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

Arrayが本当に配列なら for in はだめ
そうじゃないなら名前がだめか、Docにコメントつけたほうがいい。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

ググッたら、連想配列は for in って事らしかとたんだけど、違うのすかね?

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

連想配列の名前にArrayってつけたら普通の配列の名前にはなんてつけるんや・・・。
あとJSには連想配列としてデザインされてるものはない、連想配列のように使えるだけのObject。

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

配列(Array)と連想配列(Object)の違いを勉強するお

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

連想配列と配列が、型が違うって知りませんでした!!!!
PHP と違うんですね!!!!

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

変数名直します!!!!

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

ArrayとObjectの違いを厳密に考えだすと、JSの深淵に連れて行かれる。
Arrayのインスタンスをfor inするとリスクがあるってことだけ覚えといてくれればArrayを for in してもいいっす。

function postNice(){
var issueCommentId = this.dataset.issueCommentId;
// いいねボタンを一瞬黄色にする
var buttonImgElm = this.querySelector('img');

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

こういう文脈でのthisはだめ。トラブルの元。
"use strict"したら怒られるし。

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

thisの代わりにどうしたら良いのかはググれ!

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

thisこわい((((;゚Д゚))))ガクガクブルブル

This comment has been minimized.

Copy link
@nezumi650

nezumi650 May 5, 2013

Author Owner

あれこれ直したつもりだったけど、firefox で動いてなかった。
どうやって直せばいいんだ。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 May 5, 2013

Author Owner

firefox で動く様にした、use strict で怒られないから、この方法でいいんだよねきっと。。。


/**
* いいねボタンの色の変更
*/

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

class名の付け替えであたるCSSが変わるようにして色変えたほうがいい。
あと色なんかの名前が直接JSに出てるのはよくない、まして変数・関数名はやばい。
デザイン変更されたら、関数名変わるじゃん。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

確かにその通りですね!!!!
修正します!!!!

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

確かにchangeYelloはやばい感ただよう

This comment has been minimized.

Copy link
@gilbite

gilbite Apr 11, 2013

いやこれは「黄色」というのが共通言語になっているならいいと思う。
ほら、信号も緑だけど「青」っていうし。
「黄色」っていうことに特別な意味があればこれはいいんだと思う。

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

あればな。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 12, 2013

Author Owner

なるほど。
しかし今回は、「黄色」にとくに特別な意味を持たせているつもりは無いので、関数名変更します!

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 13, 2013

Author Owner

なおした

@umezo

This comment has been minimized.

Copy link

commented Apr 11, 2013

グリモンだったら全然きにしないけど、っていうレベルの指摘をした。
でも仕事で書く時に気をつけてるレベルではある。

var commentForm = document.querySelector( '[id^="comment_body_"]' );
commentForm.value = '<p class="IINEComment"> ' + target + ' ' + customComment + ' ' + '<br />' + defaultMessage + repoURL + '</p>';

var submitButton = document.querySelector('.form-actions:last-child button[type="submit"]:last-child');

This comment has been minimized.

Copy link
@gilbite

gilbite Apr 11, 2013

おいこインデントのずれはなんだ

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

一緒に仕事したくないレベル

This comment has been minimized.

Copy link
@haruton

haruton Apr 11, 2013

くそわろた

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

正直すみませんでした


// ちょっと待たないと書き込み終わってなくてhideできないので苦肉の対応
// @TODO もうちょっとかっこ良くしたい。。。。
setTimeout( function(){

This comment has been minimized.

Copy link
@gilbite

gilbite Apr 11, 2013

どうなってんだ

This comment has been minimized.

Copy link
@umezo

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

正直すみませんでした(インデントだよね?

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 11, 2013

Author Owner

あれ、インデントの話じゃない?

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

これってsetTimeoutで遅延させてるのがって話なのかとおもた。
もしsetTimeoutを変えたいならDOMの挿入イベントを監視するのをググるといいかも。(あったような気もするけど、なかったような気もするw

@umezo

This comment has been minimized.

Copy link

commented Apr 11, 2013

全体的なはなし。
生のJSは型が緩すぎるので少なくとも引数の型はコメントなりなんなりで絶対に明示したほうがいい。
自分ですらメンテ出来なくなるし、書いてない人間は実行するまで型分からない。

var originalCommentId = matches[0];
} else {
var originalCommentId = 'issue';
}

This comment has been minimized.

Copy link
@haruton

haruton Apr 11, 2013

use strict するとこのへんもエラーになるの?

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

やったら分かることを質問しちゃーなんねぇ

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 14, 2013

Author Owner

何も起こらないお。。。

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Apr 14, 2013

Author Owner

firefox で検証したら、めっちゃ色々出てました。
色々直します。


// HTMLに入れ込む
var issueElm = document.querySelector( '[id^="issue-"]' );
var commentHeaderElm = issueElm.querySelector('.discussion-topic-author');

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

querySelectorかっこいいいいいいいいいいいい

changeYellow(buttonImgElm);
postComment('This issue');
// いいねボタンを白色に戻す
setTimeout( function(){changeDefault(buttonImgElm)}, 300);

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

こんなふうにもかけますよ〜。

setTimeout( changeDefault.bind(this, buttonImgElm), 300);

bindは関数を特定のコンテキストに束縛する機能があって、第2引数以降には束縛したい引数も渡せます。
モダンブラウザは対応してると思いますが、IEはしらないですw

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

IEが気になったらjQueryさんの出番で・・・。$.proxy

@h13i32maru

This comment has been minimized.

Copy link

commented Apr 11, 2013

@umezo
knockout.jsってどう思いますか?
機能がシンプルで使いやすいなーと思ってるんですが、もし評価してたらどんな感じか教えて欲しいです。
僕はwith, if, foreachまわりのcontextではまりました。(コメントスタイルとdat-bindスタイルで動作がちがうあたり
(本人に了承を得て違う話をPRに関係ない話をかいてますw

*/
function deleteIINEComment() {
var commentId = this.getAttribute('for');
// console.log(commentId);

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

ログはけすべし

function addIINEIcon(discussionBubblesAllArray) {

for (var parentId in discussionBubblesAllArray){
if (parentId == 'issue') {

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

暗黙の型変換による罠に気をつけるためにも===をおすすめします

var avatarArea = parentElm.querySelector('.avatarArea');
if (avatarArea) {
// すでにあれば、一回中身を削除
avatarArea.innerHTML = avatarSettingDefaultInnerHTML;

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

innerHTMLはXSSの脆弱性に気を使う必要があるから、できるならtextContentがいいと思うすー。
でもテキストじゃなくてHTMLをぶっこみたいならDOM構築がんばるんだ

https://developer.mozilla.org/ja/docs/DOM/Node.textContent

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

レビュー漏れてた。 @h13i32maru のいうう通り。

This comment has been minimized.

Copy link
@umezo

umezo Apr 11, 2013

あ。あとIEでtextContentは(略

var commentId = discussionBubblesArray[i].querySelector('[id^="issuecomment-"]').getAttribute('id');
var avatarImgElm = discussionBubblesArray[i].querySelector('.discussion-bubble-avatar');
// 小さいアバター画像を作成
var addAvatarImg = document.createElement('img');

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

addって書いてあるとメソッド名っぽい・・・

// まだ無ければ作成
avatarArea = document.createElement('p');
avatarArea.style.textAlign='right';
avatarArea.className='avatarArea';

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

=の前後にはスペースを入れるか入れないかはっきりしてよ!

}

var discussionBubblesArray = discussionBubblesAllArray[parentId];
for (var i = 0; i < discussionBubblesArray.length; i++) {

This comment has been minimized.

Copy link
@h13i32maru

h13i32maru Apr 11, 2013

forのなかでfor、これはイテレータの変数ではまりそうなパターンや。
こういう場合は変数名を固有でつけたほうがいいす。
(ブロックスコープがないゆえ)

@umezo

This comment has been minimized.

Copy link

commented Apr 11, 2013

全然調べてないから何も言えないす。すいません。専用記法すぎるところが嫌ってぐらいの感想しかない。
パフォーマンスが気にならなくて専用記法が自分に合ってたらいいんじゃね。
ただし何選ぶにしろデータバインディングはあったほうが設計に一定の強制力をもたせられるからいい。

個人的にはライブラリはjqueryとskin.jsぐらいで、イベントとかバインド周りは自分の管理下においておきたい。

@h13i32maru

This comment has been minimized.

Copy link

commented Apr 11, 2013

ほほー、そうなんすね。あざます。
今のところjQueryはほとんど使わずにmodelとviewを分離できてるので満足感はあります。
あと、覚えることも少ないので、初心者にも書いてもらえてます。
(skin.jsはしらないのでぐぐておきます。聞くと怒られそうなのでw

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Apr 11, 2013

とりあえず一カ所インデント直したよ!!!!!!!!!111
なんかユーザーが変になってたし、文字化けしたので、config直します!!!!

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented May 5, 2013

ステルスにポストされるコメントがスパムっぽくていけてないと、弊社スーパーウェブアーティスト・鹿氏に言われたので、なんかちょっとスパム感なくす事にした

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented May 5, 2013

issuecomment-17447999 StealthPosted. テストテスト

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented May 5, 2013

とりあえず、今回のプルリクはやりきった感出してもいいんじゃないかな


/********************************************
*** ミニアバター画像設定
********************************************/

This comment has been minimized.

Copy link
@nezumi650

nezumi650 Aug 25, 2013

Author Owner

テスト

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

ほげテストほげテストほげテスト

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

ほげテストほげテストほげテスト

2 similar comments
@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

ほげテストほげテストほげテスト

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

ほげテストほげテストほげテスト

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

Stealth Liked:
> ほげテストほげテストほげテスト

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

Stealth Liked:

ほげテストほげテストほげテスト

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

Stealth Liked:

  • // ステルスポストボタンを白色に戻す
@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

Stealth Liked:

  • for (var parentId in discussionBubblesAllArray){
@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

Stealth Liked:

  • for (var parentId in discussionBubblesAllArray){
  • var discussionBubblesArray = discussionBubblesAllArray[parentId];
  • for (var i = 0; i < discussionBubblesArray.length; i++) {
  •  discussionBubblesArray[i].style.display = 'none';
    
  • }
  • }
@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

Stealth Liked:

おい、へんなアカウント名でてるぞ

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

Stealth Liked by undefined:

マルちゃんのつっこみ冴えてる。

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 25, 2013

Stealth Liked by nezumi650:

定数名になんか規約ないの、よみずらい。

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked by nezumi650:

おい、インデントとログの削除だけやないか!

1 similar comment
@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked by nezumi650:

おい、インデントとログの削除だけやないか!

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked:

Stealth Liked by nezumi650:

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked:

firefox で検証したら、めっちゃ色々出てました。

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked:

100理ぐらいあって1害もないからやっとけ!

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked:

You are receiving notifications because you authored the thread.

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked:

© 2013 GitHub, Inc.

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked:

JSLintが厳しすぎたらJSHintでもいいのよ。

@nezumi650

This comment has been minimized.

Copy link
Owner Author

commented Aug 26, 2013

Stealth Liked:

グリモンだったら全然きにしないけど、っていうレベルの指摘をした。
でも仕事で書く時に気をつけてるレベルではある。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.