javascriptの精査をお願いしたいです。勉強中です。

javascriptでタブ機能を作成してみました。
なんとか動くところまでもっていくことができましたが、
恐らくとても非効率な書き方をしていると思います。

もしよろしければ、こうしたほうがいいよとか、自分だったらこうする的なご意見をいただきたく投稿させていただきました。
よろしくお願いいたします。

<!doctype html>
<html>
<head>
<meta charset="UTF-8">
<title>タブ</title>
</head>
<body>

<style>
* {
margin: 0;
padding: 0;
}

body {
padding: 100px;
}
.tabList {
display: table;
width: 500px;
margin: 0 auto;
}
.tabList li {
display: table-cell;
border-top: 1px solid #ccc;
border-bottom: 1px solid #ccc;
border-left: 1px solid #ccc;
}
.tabList li:last-child {
border-right: 1px solid #ccc;
}
.tabList li a {
display: block;
padding: 5px 0;
text-align: center;
text-decoration: none;
transition: 0.2s;
}
.tabList li a:hover {
background: #ccc;
color: #fff;
transition: 0.2s;
}
.tabContent {
width: 500px;
margin: 0 auto;
margin-bottom: 50px;
}
.tabContent .tabPanel {
padding: 10px;
border-right: 1px solid #ccc;
border-bottom: 1px solid #ccc;
border-left: 1px solid #ccc;
}
.layout {
display: grid;
grid-template-columns: 50px 1fr 50px;
background: #ccc;
}
</style>

<ul class="tabList">
<li>
<a href="#tab01">タブ01</a>
</li>
<li>
<a href="#tab02">タブ02</a>
</li>
<li>
<a href="#tab03">タブ03</a>
</li>
</ul>
<div class="tabContent">
<div id="tab01" class="tabPanel">
<p>タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01</p>
</div>

<div id="tab02" class="tabPanel">
<p>タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02</p>
</div>

<div id="tab03" class="tabPanel">
<p>タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03</p>
</div>
</div>


<ul class="tabList">
<li>
<a href="#tab04">タブ01</a>
</li>
<li>
<a href="#tab05">タブ02</a>
</li>
<li>
<a href="#tab06">タブ03</a>
</li>
</ul>
<div class="tabContent">
<div id="tab04" class="tabPanel">
<p>タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01タブパネル01</p>
</div>

<div id="tab05" class="tabPanel">
<p>タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02タブパネル02</p>
</div>

<div id="tab06" class="tabPanel">
<p>タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03タブパネル03</p>
</div>
</div>

<script>
window.addEventListener('DOMContentLoaded', function () {
    const tabContent = document.getElementsByClassName('tabContent');
    const tabList = document.getElementsByClassName('tabList');
    let ids = [];
    for (let i = 0; i < tabContent.length; i++) {
        var tabPanel = tabContent[i].getElementsByClassName('tabPanel');
        for (let j = 0; j < tabPanel.length; j++) {
            tabPanel[j].style.display = "none"; //tabPanel全部非表示
            tabPanel[0].style.display = "block"; //一番目のtabPanelだけ表示
        }
    }
    for (let i = 0; i < tabList.length; i++) {
        const trg = tabList[i].getElementsByTagName('a');
        for (let j = 0; j < trg.length; j++) {
            trg[j].addEventListener('click', function () {
                //tabList.getElementsByTagName('li').classList.remove("active");
                this.parentElement.classList.add("active");
                console.log(trg);
                const nextTabPanel = this.parentElement.parentElement.nextElementSibling.getElementsByClassName('tabPanel');//クリックした要素の親要素を取得
                for (let k = 0; k < nextTabPanel.length; k++) {
                    var href = this.getAttribute('href').replace('#', '');//クリックした要素のhref属性を取得して#を削除
                    nextTabPanel[k].style.display = 'none';//すべてのパネルを日商事にする
                    const id = nextTabPanel[k].getAttribute('id'); //各idを取得
                    ids[k] = id; //配列に格納
                }
                let num = ids.indexOf(href);
                nextTabPanel[num].style.display = 'block';
            });
        }
    }
});
</script>
</body>

</html>

masaya_ohashi

2017/11/09 00:49

これは「JavaScriptの練習の為」に自前でタブ機能を素のJavaScriptだけで作ったということでしょうか？

退会済みユーザー

2017/11/09 00:51

そうです！

masaya_ohashi

2017/11/09 01:11

IEで動く必要はありますか？IEは結構古い書き方しかサポートしていないので、最新の便利な機能が使えないことが多いです。

退会済みユーザー

2017/11/09 01:26

IE10くらいで考えていましたが、練習なので最新対応でも問題ありません。ただある程度古い書き方も学んだほうがいいのかと思い、querySelectorだけあえて使ってませんでした。周りにjsができる人がいないので、自分の書き方をレビューしていただけれるなら最新の書き方でも大丈夫です。

行動規範の内容に同意します

回答5件

勉強ということなので、
本気でレビューしてみました。

ES2015のコードは() => {}というアロー関数が使えます。

（強制はしませんが、一部の処理はささっと書けるようになります）

document.getElementsByClassNameではなく、

CSSセレクタが使えるdocument.querySelectorAllの方が直感的でしょう。

1回のfor文でやりたい事を詰め込みすぎです。

それでは不具合修正等で地獄を見ます。
パフォーマンスに酷い悪影響が出ない限りは1ループにつき1個の事に集中しましょう。

HTMLCollectionで配列メソッドを使うというイディオムがあります。これを使ってリスト操作の関数を使用しましょう。
見た目を変更する場合、クラスを付け外ししたいのか、要素のCSSプロパティを直接いじりたいのか、一環した動作にしましょう。

非表示のにするかわりにinvisibleといったクラスを用意して、display: none;を指定するCSSプロパティを振り分けるのがおすすめです。

変数名は単数形、複数形をちゃんと使い分けましょう。

getElementsなのにtabContentでは配列か要素1個か変数名からは判断出来ませんからね。

a要素のhref属性で頑張ってますがHTML5ならばdataを使えば好きな情報を好きな要素に格納できます。li要素のみで十分でしょう

※リスト操作用のArray.prototype.xxxメソッド達

map: 全ての要素に1個ずつ関数を適用していき、戻り値を使って同じ要素数の配列を生成しなおす
forEach: mapと同様だが戻り値は捨てる。処理したいだけの時はこちらを使うとコードを見返す時に分かりやすい
filter: mapと同様、Booleanを返す関数に適用していき、結果がtrueのものだけの配列を作り直す

ちょっとさわりの部分だけリファクタリングしてみました。
配列操作関数を使って一気に処理する流れに変更したので、
i, j, kといった状態変数の管理がなくなってぐっとわかりやすくなったかと思います。

JavaScript
1const nodes = (selector, target = document) => {
2    const nodeList = target.querySelectorAll(selector);
3    return Array.prototype.slice.call(nodeList);
4}
5const node = (selector, target = document) => {
6    return target.querySelector(selector);
7}
8
9window.addEventListener('DOMContentLoaded', () => {
10    // 先頭のtabPanel以外を非表示に変更
11    // tabはつまみやでっぱりの部分を指す単語なので、変数名はpanelに変更
12    const panels = nodes('.tabContent .tabPanel');
13    panels.forEach(it => it.classList.add('invisible'));
14    panels[0].classList.remove('invisible');
15
16    // タブクリック時の挙動
17    const tabs = nodes('.tabList li');
18    tabs.forEach(tab => {
19        node('a', tab).addEventListener('click', () => {
20            // クリックしたタブのみをアクティブに変更し他はアクティブを解除
21            tabs.forEach(it => it.classList.remove('active'));
22            tab.classList.add('active');
23
24            // ここから先は自分の目で確かめるんだ！！！
25        });
26    });
27});

投稿2017/11/09 02:13

miyabi-sun

総合スコア21158

miyabi-sun

2017/11/09 02:41

あ〜…そういやquerySelectorAllで返すNodeListはforEach持ってましたね。 IE11だとNodeListが何故かforEach持ってないのでArrayに変換して使ってます。アロー演算子等を豪勢に使ってるのでどの道IE11では動かず中途半端な対応だったかもしれません。

退会済みユーザー

2017/11/09 02:43

本気のレビューありがとうございます。こういった細かい指摘は心からありがたく思います。調べようと思ってもまず何を調べたらいいのかわからないのが現状で、変数名やfor文の使い方など基礎的な部分での指摘はとても嬉しいです。質問を投げた甲斐がありました。参考にさせていただきます。

masaya_ohashi

2017/11/09 02:56

ほんとIEは早く滅んでほしいですね。

miyabi-sun

2017/11/09 03:31 編集

ほんとIE対応しない世界になって欲しいですね。 Windows7のサポート終了は2020年らしいけど、 Windows10もIE11搭載しててサポート終了が2025年…おじいちゃんになっちゃぅぅぅ

root_jp

2017/11/09 03:16

2025年。。。長すぎ！！

行動規範の内容に同意します

ベストアンサー

querySelectorを使わない書き方で地道にやるのはやや時代に取り残されすぎかと思います。包丁が使えるのに未だに石器を使って練習するのと同じくらいです。確かに練習としてはよいかもしれませんが、すでに「使わなくても書けるやりかたを知っている」のであれば、これ以上使わないで書き続けて練習することもないかと思います。

querySelectorやclosestをフルに使って書いてみました。
ポイントは「:first-child」による指定、「初期化をclick発火で済ます」といった点でしょうか。

JavaScript
1// tabList内のaタグ全て
2document.querySelectorAll('.tabList a').forEach(function(elem, index) {
3  // クリックイベント処理の登録
4  elem.addEventListener('click', function(e) {
5    // aタグをクリックしたときに画面が移動してしまうことを回避
6    e.preventDefault();
7    
8    const href = this.getAttribute('href'); // クリックされたaタグのhref
9    const parentTabList = this.closest('.tabList'); // クリックされたaタグの属するtabList
10    
11    // 一旦同tabList内のliを全て非アクティブにして
12    parentTabList.querySelectorAll('li').forEach(function(elem, index) {
13      elem.classList.remove('active');
14    });
15    
16    // 親liだけを改めてアクティブ化
17    this.closest('li').classList.add('active');
18    
19    const tabPanel = document.querySelector(href); // 対象のtabPanel
20    const parentTabContent = tabPanel.closest('.tabContent'); // 対象のtabPanelの属するtabContent
21    
22    // 一旦対象のtabContent内のtabPanelを全て非表示にして
23    parentTabContent.querySelectorAll('.tabPanel').forEach(function(elem, index) {
24      elem.style.display = 'none';
25    });
26    // 対象のtabPanelだけ表示状態にする
27    tabPanel.style.display = 'block';
28  });
29});
30
31// 各tabListの先頭li内のaタグをクリックしたことにする
32// これにより上記click時の処理を再利用して初期状態にすることができる
33document.querySelectorAll('.tabList li:first-child a').forEach(function(elem, index) {
34  elem.click();
35});
36

【動作サンプル】
https://jsfiddle.net/fker4dng/

投稿2017/11/09 01:34

編集2017/11/09 01:35

masaya_ohashi

総合スコア9206

退会済みユーザー

2017/11/09 01:45

わざわざソースまで提示していただきありがとうございます！正直何を書いてあるのかすぐには理解できませんが一個ずつ読み解いていきたい思います。 querySelectorに関しては確かに意味もなく意地になっていた気がします。見てくれる人がいなかったので指摘されてやっと踏ん切りがつきました。今後は使えるものは使って行きたいと思います。ありがとうございました。

miyabi-sun

2017/11/09 02:24

closestは初見でした。めっちゃ便利ですね！

masaya_ohashi

2017/11/09 02:29

parentNodeだと階層が変わってしまうと困りますし、parentNodeを遡り続けて検索をわざわざコーディングするの大変ですからねぇ。closestにはお世話になっています。

行動規範の内容に同意します

あんまりやってることは変わらないんですけど、多少短くなったかなと思います。

JavaScript
1window.addEventListener("DOMContentLoaded", function() {
2  // 初期表示
3  document.querySelectorAll("div.tabContent").forEach(tabContent => {
4    tabContent.querySelectorAll("div.tabPanel:nth-of-type(n + 2)").forEach(tabPanel => tabPanel.style.display = "none");
5  });
6
7  // タブにクリックイベント付与
8  document.querySelectorAll("ul.tabList a").forEach(tab => {
9    tab.addEventListener("click", event => {
10      event.preventDefault() // アンカークリックによる画面移動を回避
11      const targetTabPanel = document.querySelector(event.target.hash);
12      targetTabPanel.parentElement.querySelectorAll("div.tabPanel").forEach(tabPanel => {
13        // 対象のものだけを表示。他は非表示。
14        tabPanel.style.display = tabPanel === targetTabPanel ? "block" : "none";
15      });
16    });
17  });
18});
19

投稿2017/11/09 03:12

編集2017/11/09 03:31

root_jp

総合スコア4666

元も子もない話かもしれませんが，UIは既にあるものを使ったほうがいいと思います．
そのほうが簡単かつ綺麗ですし，違う部分に勉強の労力を割くことができます．
（もし細かくいじりたい場合でも，既存のものをいじったほうがいいのでは）

https://codepen.io/Yatima/pen/xPRvav
こちらはVue.jsとVue Materialを利用したサンプルです．

投稿2017/11/09 01:07

Yatima

総合スコア1159

退会済みユーザー

2017/11/09 01:35

ありがとうございます！おっしゃるとおりだと思います。 jQueryなら多少はできるので、素のjavascriptを書くことが効率的でないのことは重々承知しております。ただやはりライブラリに頼る前に、javascriptで基礎を学んだほうが後々自分のためになるのではと思っています。 Vue.jsも基礎が身についてからやっていきたと思います。

行動規範の内容に同意します

既に解決済みですが、『色んな書き方がある』っていう部分で参考になるところがあればと思いコメントさせてもらいました。
まだコードとして直したいと思う部分もありますが、私が意識したのは『同じような処理を行うものについては関数化してそれをうまく使いまわす』というイメージでしょうか。
とは言え、そこに意識が偏り過ぎて無駄に引数を使って関数自体が複雑になるのは良くないと思うので、簡易的な処理だけにする。という事が大事だと思っています。

javascript
1  const tabContent = document.querySelectorAll('.tabContent');
2  const tabPanel = document.querySelectorAll('.tabPanel');
3  const ulTabList = document.querySelectorAll('.tabList a');
4
5  const controlPanel = () => {
6    // ループ処理のみ行う関数
7    const eachMethod = (target, callback) => {
8      target.forEach(elm => {
9        callback(elm);
10      });
11    };
12
13    // コンテンツ表示させるための関数
14    const contentsView = (parentElm, obj) => {
15      eachMethod(parentElm, elm => {
16        if (obj.idx !== null) {
17          elm[obj.target][obj.idx].classList[obj.method]('d-none');
18          return;
19        }
20        elm.classList[obj.method]('d-none');
21      });
22    };
23
24    // 全てのタブの中身を非表示
25    eachMethod(tabPanel, iElm => {
26      iElm.classList.add('d-none');
27    });
28
29    // タブの親要素を取得して子要素を処理する関数へ引き渡す
30    eachMethod(tabContent, () => {
31      const objData = {
32        target: 'children',
33        idx: 0,
34        method: 'remove'
35      };
36      contentsView(tabContent, objData);
37    });
38
39    // タブクリックでコンテンツの表示を切り替える
40    eachMethod(ulTabList, iElm => {
41      iElm.addEventListener('click', () => {
42        const openContents = document.querySelector(iElm.hash);
43        const parentOpenContents = openContents.closest('.tabContent');
44        const eachTarget = parentOpenContents.children;
45        const callValue = Array.prototype.slice.call(parentOpenContents.children);
46        const objData = {
47          target: null,
48          idx: null,
49          method: 'add'
50        };
51        contentsView(callValue, objData);
52        openContents.classList.remove('d-none');
53      }, false);
54    });
55  };
56
57  window.addEventListener('DOMContentLoaded', () => {
58    controlPanel();
59  }, false);