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

fix: 「Todoアイテムの追加を実装する」の章で、ulを追加 #1385

Merged
merged 9 commits into from
Feb 9, 2022

Conversation

kupuma-ru21
Copy link
Contributor

#1316
上記、issueの対応を致しました。

以下、対応後の表示確認です。

  • chrome
chrome.mov
  • firefox
firefox.mov

@azu azu self-requested a review February 6, 2022 11:23
@bot-user
Copy link

bot-user commented Feb 6, 2022

✔️ Deploy Preview for js-primer ready!

🔨 Explore the source changes: 585b6e2

🔍 Inspect the deploy log: https://app.netlify.com/sites/js-primer/deploys/620330ae51f2640007a14472

😎 Browse the preview: https://deploy-preview-1385--js-primer.netlify.app

@azu
Copy link
Collaborator

azu commented Feb 6, 2022

PRありがとうございます!

// ulはない
cy.get("#js-todo-list ul").should(items => {
expect(items).to.have.length(0);
});

このテストも修正が必要っぽいですね。

            // ulがある
            cy.get("#js-todo-list ul").should(ul => {
                expect(ul).to.have.length(1);
            });

になるかなと思います。

@azu
Copy link
Collaborator

azu commented Feb 6, 2022

このセクションではまだ利用しませんが、`html-util.js`には`render`という関数を定義しています。

render関数を使っているので、ここは削除する必要がありますね

最後に、この`element`タグ関数を使って、フォームから送信された入力内容をTodoリストに要素として追加してみます。
`App.js`から先ほど作成した`html-util.js``element`タグ関数を`import`します。
次に`submit`イベントのリスナー関数で、Todoアイテムを表現する要素を作成し、Todoリスト(`#js-todo-list`)の子要素として追加(`appendChild`)します。
最後にTodoアイテム数(`#js-todo-count`)のテキスト(`textContent`)を更新します。

ここの文章も変更が必要ですね。
2ステップが3ステップになって少し長くなるので、文もちょっと分けたほうが良さそうですね。

最初に大きく分けて何をやるか概要を出して、その中身を説明する感じですかね。

  • Listへの追加と描画
  • カウントの更新
最後に、この`element`タグ関数と`render`関数を使って、フォームから送信された入力内容をTodoリストに要素として追加してみます。

まず最初に、`App.js`から先ほど作成した`html-util.js`の`element`タグ関数と`render`関数を`import`します。
次に、Todoアイテムをまとめるリストを`todoListElement`として定義し、表示されているTodoアイテムの数を`todoItemCount`として定義します。

`submit`イベントのリスナー関数で、入力された内容をもとにTodoリストとTodoアイテム数の表示を更新していきます。
Todoアイテムを表現する要素を作成し、Todoリストの子要素として追加(`appendChild`)して、`render`関数でコンテナ要素の中身をTodoリストで上書き(再描画?)します。
最後にTodoアイテムの数を1増やして、Todoアイテム数(`#js-todo-count`)のテキスト(`textContent`)を更新します。

イメージはこういう感じになるのかなと思います。(ちょっと書いてみて読みやすいかの確認が必要そう)
ちょっとカッコが多くなりがちなので、ちょっと意図的に抑えないといけない感じがしますね…

@azu azu mentioned this pull request Feb 7, 2022
@kupuma-ru21 kupuma-ru21 closed this Feb 7, 2022
@kupuma-ru21 kupuma-ru21 reopened this Feb 7, 2022
@kupuma-ru21
Copy link
Contributor Author

kupuma-ru21 commented Feb 7, 2022

#1385 (comment)

このテストも修正が必要っぽいですね。

修正例を記載していただき、ありがとうございます!
修正いたしました。

de1e4c1

@kupuma-ru21
Copy link
Contributor Author

kupuma-ru21 commented Feb 7, 2022

#1385 (comment)

このセクションではまだ利用しませんが、`html-util.js`には`render`という関数を定義しています。

render関数を使っているので、ここは削除する必要がありますね

すみません、確認不足でした。
文言を削除いたしました。

2f95767


最後に、この`element`タグ関数を使って、フォームから送信された入力内容をTodoリストに要素として追加してみます。
`App.js`から先ほど作成した`html-util.js``element`タグ関数を`import`します。
次に`submit`イベントのリスナー関数で、Todoアイテムを表現する要素を作成し、Todoリスト(`#js-todo-list`)の子要素として追加(`appendChild`)します。
最後にTodoアイテム数(`#js-todo-count`)のテキスト(`textContent`)を更新します。

イメージはこういう感じになるのかなと思います。(ちょっと書いてみて読みやすいかの確認が必要そう)
ちょっとカッコが多くなりがちなので、ちょっと意図的に抑えないといけない感じがしますね…

こちら、記載していただいた文章をもとに修正いたしましたので
確認して頂ければと思います。

cfd11b2

@@ -67,8 +67,6 @@ body {
font-size: 24px;
border-bottom: 1px solid #ededed;
padding: 16px;
/* https://github.com/asciidwango/js-primer/issues/1316 */
list-style: none;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📝 書籍版からも参照してるので、書籍版では liの問題は再発はするけど、リリース時もそういう感じだったので、まあいいかな。

Copy link
Collaborator

@azu azu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTMです!

ありがとうございました!

@azu azu merged commit c24521b into asciidwango:master Feb 9, 2022
@kupuma-ru21
Copy link
Contributor Author

LGTMです!

ありがとうございました!


確認していただき、ありがとうございました!

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.

None yet

3 participants