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

refactor(ajaxapp): データを表示する/Promiseを活用する のリファクタリング #914

Merged
merged 5 commits into from
Aug 3, 2019

Conversation

azu
Copy link
Collaborator

@azu azu commented Aug 1, 2019

No description provided.

<script src="index.js"></script>
</body>
```
[import, title:"index.html"](./src/index.html)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

あえて部分を表示する必要も無いかなと

@bot-user
Copy link

bot-user commented Aug 1, 2019

Deploy preview for js-primer ready!

Built with commit 0a9f5c8

https://deploy-preview-914--js-primer.netlify.com

@azu azu changed the title refactor(ajaxapp): データを表示する のリファクタリング refactor(ajaxapp): データを表示する/Promiseを活用する のリファクタリング Aug 1, 2019
@@ -121,6 +108,14 @@ function fetchUserInfo(userId) {

### Promiseチェーンのリファクタリング {#refactor-promise-chain}

<!-- textlint-disable ja-technical-writing/sentence-length -->

現在の`fetchUserInfo`関数は、HTMLの組み立て(`createView`)と表示(`displayView`)も行っています。
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Promiseチェーンの説明から入ると結局どうしたいのか伝わりにくい感じがした。
なので、最初になにをするかを宣言するようにしてから、Promiseチェーンの説明という流れに変えた

@azu azu requested a review from lacolaco August 1, 2019 14:43

## HTMLを組み立てる {#markup-html}

HTML文字列の生成にはテンプレートリテラルを使います。
テンプレートリテラルは文字列中の改行が可能なため、HTMLのインデントを表現できて見通しが良くなります。
また、文字列の埋め込みも簡単なため、HTMLのテンプレートに対して動的なデータを当てはめるのに適しています。
また、変数の埋め込みも簡単なため、HTMLのテンプレートに対して動的なデータを当てはめるのに適しています。
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

埋め込むのは変数?

@@ -7,7 +7,7 @@
<h2>GitHub User Info</h2>

<button onclick="fetchUserInfo('js-primer-example');">Get user info</button>

<!-- 整形したHTMLの挿入先 -->
<div id="result"></div>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Todoアプリに合わせるなら #js-resultだけど、あっちはクラスがでてくるかというのもあるか。
prefixつけるとここで説明でてきちゃうからこのままで

@@ -39,6 +39,7 @@ GitHubのAPIに対してHTTPリクエストを送信しましたが、まだレ
また、`Response`オブジェクトの`json`メソッドも`Promise`を返します。これは、HTTPレスポンスボディをJSONとしてパースしたオブジェクトでresolveされます。
ここでは、書籍用に用意した`js-primer-example`というGitHubアカウントのユーザー情報を取得しています。

{{book.console}}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

一応ブラウザで実行はできるのでつけた

@azu
Copy link
Collaborator Author

azu commented Aug 1, 2019

diffはdiffとして表現する必要性があるきがした。
全体を書き換えるのか一部を書き換えるのかがわかりにくい感じがした

<!-- textlint-disable ja-technical-writing/sentence-length -->

現在の`fetchUserInfo`関数は、HTMLの組み立て(`createView`)と表示(`displayView`)も行っています。
`fetchUserInfo`関数に処理が集中しているため見通しが悪くなるため、`fetchUserInfo`関数はデータの取得だけを行うように変更します。
Copy link
Collaborator

@lacolaco lacolaco Aug 2, 2019

Choose a reason for hiding this comment

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

〜ため〜ため は避けたい?

Copy link
Collaborator

Choose a reason for hiding this comment

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

集中していて かな

Copy link
Collaborator

@lacolaco lacolaco left a comment

Choose a reason for hiding this comment

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

LGTMです。diffの件も同意ですがどう表現すると良いんでしょうね...

@azu
Copy link
Collaborator Author

azu commented Aug 3, 2019

TODOだと // ...省略... みたいな感じの入れてた
https://jsprimer.net/use-case/todoapp/event-model/

.js コードとしては全体書いておいて、
https://github.com/azu/gitbook-plugin-include-codeblock#snippet-code 一部を切り出すとかするとテストもしやすいかなー
E2Eテスト的に差分も一つのソースとして扱う感じ。 #911

さて、今の`fetchUserInfo`関数ではFetch APIが返したPromiseの`then`でHTMLの組み立てと表示も行っています。
このPromiseチェーンを次のように書き換えてみましょう。
`fetchUserInfo`関数では、Fetch APIが返すPromiseの`then`メソッドで、`Reponse#json`メソッドの戻り値を返しています。
`index.js`の`fetchUserInfo`関数と`main`関数を次のように書き換えます。
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

コードの説明にした。

@azu azu merged commit 446a7c0 into master Aug 3, 2019
@azu azu deleted the 2019-ajax-display-refactor branch August 3, 2019 08:39
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.

3 participants