feat: スポンサー一覧ページを作成する #170

ryamakuchi · 2019-09-01T09:01:49Z

resolve: #132

TODO

レビューポイント

デザインを正確に表現できているか
ページのメタ情報は適当か
テストケースは十分か

この PR で対応しないこと

index のインタラクション

#132 (comment)
こちらのコメントのとおり、この PR では lg サイズ以上の index のインタラクションについては対応せず、別 PR で開発を進める予定です。

参考

close: #165

デザインカンプはこちら↓

https://www.figma.com/file/BpIgcZdusbS3CgPHQhhK79T7/Vue-Fes-Japan-2019-Web?node-id=1080%3A3423

ryamakuchi · 2019-09-01T09:45:50Z

@inouetakuya @yotaszk @448jp

メタ情報のレビューをお願いします 🙏

メタ情報

    const url = `https://vuefes.jp/2019/sponsors`
    const title = 'スポンサー一覧 | Vue Fes Japan 2019'
    const description = 'Vue Fes Japan 2019 のスポンサー情報です。'

レビュー観点

スポンサー一覧ページ専用の OG 画像を用意する必要があるかどうか（現在はデフォルトの設定になっています）
- ある場合は用意していただけると助かります！
「スポンサー一覧」というタイトルは適切か

448jp · 2019-09-01T11:45:52Z

タイトル、私はいいと思います。専用OGPは不要（トップと同じでOK）です。

src/pages/sponsors.vue

…numSponsor: SponsorPlans = sponsorList[2]'

inouetakuya · 2019-09-28T12:19:34Z

sponsorPlansToHaveSponsor という名前は適切か？

ToHave だと「スポンサーを持つべき sponsorPlans」というニュアンスがある
持っているスポンサーは複数なのに ToHaveSponsor である

という点に違和感を感じました。

「スポンサーを持っている sponsorPlan の一覧を取得できる」

これをそのまま表現して sponsorPlansHavingSponsors でどうでしょう？

なお、having の箇所は SQL の Having 句にインスパイアされています。

補足

デメリット

この手の後ろから修飾する言葉は、ループで回したりするときに、多少違和感が生まれます。

例）sponsorPlansHavingSponsors.forEach(sponsorPlan => ...)

（できれば xxxSponsorPlans で終わりたいという意味）

その他の案

「〇〇を持っている」として with を使うこともあります。

sponsorPlansWithSponsors とか。

ただし、この場合には、配列の要素ひとつひとつに sponsors が含まれている印象を持たせてしまうと思います。

[
  { sponsorPlan: { ... }, sponsors: [...] },
  { sponsorPlan: { ... }, sponsors: [...] },
  { sponsorPlan: { ... }, sponsors: [...] },...
]

というものが帰って来そうな印象を受けます。今回のように「絞り込む」用途では having を使う方がよいと思います。

inouetakuya

不安な部分

を書いてくださっていたのでレビューしやすかったです 👍

当該箇所にコメントしたので確認してください〜 ☺️

inouetakuya · 2019-09-28T12:56:26Z

src/components/TheSponsorListSection.vue

            :key="sponsor.sys.id"
            class="sponsor"
          >
-            <a :href="sponsor.fields.url" target="_blank" rel="noopener">
+            <nuxt-link :to="`/sponsors/#sponsor_${sponsor.sys.id}`">


HTML の id 属性が数字から始まる場合、アンカーがうまく動作しないことがあったため sponsor_ プレフィックスを入れました。

あー、なるほど 👍

そすねー、自分もどうやって知ったんだっけなあ 🤔

結論

分かんない。途中まで調べたけど結局最後まで辿り着けませんでした。

後日、分かったら共有しますね〜

以下調べたログ

RFC3986

URI については RFC 3986 で定義されていますね。

RFC 3986 - Uniform Resource Identifier (URI): Generic Syntax
https://tools.ietf.org/html/rfc3986

RFC3986 で fragment あるいは fragment identifier と表記されているので、そちらの用語で調べた方がほしい情報に辿り着きやすそうです。

URI = scheme ":" hier-part [ "?" query ] [ "#" fragment ]

RFC2046

The fragment's format and resolution is therefore
dependent on the media type [RFC2046] of a potentially retrieved
representation, even though such a retrieval is only performed if the
URI is dereferenced.

という記述があり、つまり、fragment の format は RFC2046 に依存しているよ、と。

で、RFC2046 の中から、フラグメント識別子のフォーマットに関する記述を探してみたのですが、全然わからねえ...

src/store/sponsors.ts

inouetakuya · 2019-09-28T12:58:27Z

test/unit/specs/store/sponsors.spec.ts

+          }
+        }
+        expect(
+          // Getters の型を備えたスタブを準備するのが大変でコストに見合わないため @ts-ignore


ゲッターをモックするケース

第2引数として利用するゲッターが（Store から利用する場合に引数なしで）値を返す場合はモックしたほうがハンドリングしやすいと思います。

Vuex 公式ドキュメントにはその例が載っていますね。

https://vuex.vuejs.org/ja/guide/testing.html#%E3%82%B2%E3%83%83%E3%82%BF%E3%83%BC%E3%81%AE%E3%83%86%E3%82%B9%E3%83%88

その他の例

https://lmiller1990.github.io/vue-testing-handbook/ja/vuex-getters.html#%E3%83%86%E3%82%B9%E3%83%88%E3%82%92%E6%9B%B8%E3%81%8F

ゲッターをモックしないケース

一方で、今回のケースのように、第2引数として利用するゲッターが（Store から利用する場合に）引数を要する関数の場合は、モックしないほうが「第2引数のゲッターの内容を変更したときに自動的に追従してくれる」というメリットがあると思います。

getters.sponsorPlansToHaveSponsor(state, { sponsorsByPlan: getters.sponsorsByPlan(state) })

src/types/sponsors.ts

inouetakuya · 2019-09-28T14:22:30Z

src/types/sponsors.ts

+  }
+}
+
+export interface SponsorPlans {


あえてこのファイルの中に入れるのであれば

Suggested change

export interface SponsorPlans {

export interface SponsorPlan {

ですが（単数形にする）、この SponsorPlan は Contentful に独立したモデルとして登録しても良いくらいのものなので（アプリが既に完成しているので、今回は API 定義を変更しないほうがよいですが）、

types/sponsorPlan.ts に定義しても良いかなと思いました（自分だったらそうする）

複数形と単数形がごっちゃになってしまいすみません💦
確かに定義を分けたほうが良いですね。

こちらも下記のコミットで対応しました！ありがとうございます。

1b9e897

inouetakuya · 2019-09-28T14:27:37Z

test/unit/specs/store/sponsors.spec.ts

+      })
+
+      test('plan と一致するスポンサー情報のみ取得できる', () => {
+        const platinumSponsor: SponsorPlans = sponsorList[2]


配列についてテストしているので、下記のような感じでどうでしょう？

const platinumSponsors = [sponsorList[2]] expect(getters.sponsorsByPlan(state)('platinum')).toEqual( platinumSponsors )

ご指摘ありがとうございます！ 🙇‍♀

496e1b0

こちらのコミットで修正しました。

Co-Authored-By: INOUE Takuya <inouetakuya5@gmail.com>

…into add-sponsor-details

inouetakuya

細かい点をコメントしましたが、lazyload だけマストで対応していただいて、後は want 扱いで OK です〜（あともうひと息だ！:relaxed:

inouetakuya · 2019-09-29T10:47:35Z

test/unit/specs/pages/sponsors.spec.ts

+      ])
+    })
+
+    test('スポンサープランの詳細が表示される', () => {


inouetakuya · 2019-09-29T10:49:30Z

test/unit/specs/components/TheSponsorListSection.spec.ts

+    test('リンクがスポンサー一覧ページのアンカーになっている', () => {
+      const platinumSponsorSysId: string = sponsorList[2].sys.id
+      const platinumSponsorWrapper = wrapper.find('.platinum')
+      expect(platinumSponsorWrapper.find(RouterLinkStub).props().to).toBe(


細かいですが、

Suggested change

expect(platinumSponsorWrapper.find(RouterLinkStub).props().to).toBe(

expect(wrapper.find('.platinum').find(RouterLinkStub).props().to).toBe(

というふうに繋げて書いても良いかも（お任せします）

inouetakuya · 2019-09-29T10:52:34Z

test/unit/specs/utils/createFullStore.ts

@@ -2,12 +2,14 @@ import speakersData from '../../../fixtures/contentful/speakers'
 import timeTableSectionsData from '../../../fixtures/contentful/timeTableSections'
 import eventContainersData from '../../../fixtures/contentful/eventContainers'
 import eventContainerPartsData from '../../../fixtures/contentful/eventContainerParts'
+import sponsorsData from '../../../fixtures/contentful/sponsorList'


他と合わせて下記で良さそうです。

Suggested change

import sponsorsData from '../../../fixtures/contentful/sponsorList'

import sponsorsData from '../../../fixtures/contentful/sponsors'

inouetakuya · 2019-09-29T10:54:39Z

src/pages/sponsors.vue

+              target="_blank"
+              rel="noopener noreferrer"
+            >
+              <img


このページは画像の数が多いので lazyload にしてもらいたいです〜 🙏

inouetakuya · 2019-09-29T11:06:49Z

@ryamakuchi

あと「トップに戻る」との余白が小さすぎるようなので、ここだけマストで対応お願いします 🙏

（次の PR があるようなので lazyload はそちらに回してよいと思いました）

もう日にちが日にちなので、ここの対応だけ済ませて出しちゃいましょう〜！

…できるが production 環境では margin 0 auto に上書きされてしまう

…tentful/sponsors.ts

inouetakuya · 2019-09-29T13:20:48Z

rry さんから連絡があり、体調を崩してしまったとのことでしたので、代わりにコミットしました〜。マージします！

ryamakuchi · 2019-09-29T23:10:54Z

@inouetakuya おいちゃんさんすみません、風邪でダウンしていたところ作業していただいて大変申し訳ございませんでした！！！ 🙇‍♀ 無事マージできたようで良かったです、ありがとうございます。

wip

3b994c4

pr-triage bot added the PR: unreviewed label Sep 1, 2019

ryamakuchi changed the title ~~feat: スポンサー詳細ページを作成する~~ [WIP] feat: スポンサー詳細ページを作成する Sep 1, 2019

pr-triage bot removed the PR: unreviewed label Sep 1, 2019

ryamakuchi self-assigned this Sep 1, 2019

ryamakuchi added 2 commits September 1, 2019 18:38

add pages/sponsors.vue

2a87625

path は /sponsors で不変のため余分なロジックを消した

45014e6

ryamakuchi added 2 commits September 1, 2019 21:16

Contentful からスポンサー情報を取得できるようにした

fb1cc1b

Home と同じようにスポンサー一覧を表示した

90900c7

inouetakuya added this to In progress in 本番サイト Phase 2 via automation Sep 2, 2019

inouetakuya reviewed Sep 3, 2019

View reviewed changes

src/pages/sponsors.vue Outdated Show resolved Hide resolved

ryamakuchi added 17 commits September 4, 2019 23:27

sm サイズのコーディング

c486419

md サイズのコーディング

e4229eb

アンカー以外の lg サイズのコーディング

a639a6f

アンカーをつけた

c5b824c

smooth スクロールを実装した

2f16dfb

スクロールしたときに意図した位置までいくように画像の lazyload を外した

277eb07

/sponsors の中のみスクロールするように修正

e4f8cb3

.active 以外の nav レイアウトを追加した（lg サイズ以上）

5b55351

nav でもスポンサーのいないプランは非表示になるように

fdc75ed

lg サイズ以上の全体の余白の調整

24179e7

-margin の中の計算を分かりやすく正しく直した

79caba9

.nuxt-link-active のときのスタイルを追加

349a418

lg サイズのアンカーにインタラクションをつける

7fbd178

型をつけた

1059133

トップページののリンクを /sponsors に変更した

9ec0bf2

.nav の余計な要素を表示させないように修正

add7405

yarn lint:fix

79e3ef7

fix: 'const platinumSponsor: string = sponsorList[2]' -> 'const plati…

340092f

…numSponsor: SponsorPlans = sponsorList[2]'

ryamakuchi force-pushed the add-sponsor-details branch from b1822c1 to 340092f Compare September 28, 2019 11:14

inouetakuya requested changes Sep 28, 2019

View reviewed changes

ryamakuchi and others added 6 commits September 28, 2019 23:53

fix: sponsorPlansToHaveSponsor -> sponsorPlansHavingSponsors

f07da6f

getters をモックせず使うように変更

d17e4a5

Update src/types/sponsors.ts

2b9adf2

Co-Authored-By: INOUE Takuya <inouetakuya5@gmail.com>

Merge branch 'add-sponsor-details' of github.com:kazupon/vuefes-2019 …

dc54f27

…into add-sponsor-details

SponsorPlans を SponsorPlan に名前変更してファイルに切り出した

1b9e897

sponsorsByPlan のテストを修正

496e1b0

ryamakuchi force-pushed the add-sponsor-details branch 2 times, most recently from f2bf02b to ee58d71 Compare September 28, 2019 16:04

fix: SponsorList -> Sponsor

60e9470

ryamakuchi force-pushed the add-sponsor-details branch from ee58d71 to 60e9470 Compare September 28, 2019 16:09

inouetakuya requested changes Sep 29, 2019

View reviewed changes

inouetakuya added 4 commits September 29, 2019 22:06

fix: .base-button に margin-top を設定した場合に development 環境では意図したとおりに余白が設定…

46eb930

…できるが production 環境では margin 0 auto に上書きされてしまう

スロット構文を Vue.js 2.6 以降の新しい書き方に変更した

8e8e69a

テストの書き方をカイゼンした

c88cc68

renamed: test/fixtures/contentful/sponsorList.ts -> test/fixtures/con…

44c88e0

…tentful/sponsors.ts

inouetakuya merged commit cf20720 into master Sep 29, 2019

本番サイト Phase 2 automation moved this from In progress to Done Sep 29, 2019

pr-triage bot added PR: merged and removed PR: unreviewed labels Sep 29, 2019

inouetakuya deleted the add-sponsor-details branch September 29, 2019 13:21

yinm mentioned this pull request Sep 29, 2019

グローバルメニューを作成する #181

Merged

ryamakuchi mentioned this pull request Oct 1, 2019

スポンサー一覧ページに lazyload の設定をする #190

Open

3 tasks

kazupon unassigned ryamakuchi Dec 11, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: スポンサー一覧ページを作成する #170

feat: スポンサー一覧ページを作成する #170

ryamakuchi commented Sep 1, 2019 •

edited

ryamakuchi commented Sep 1, 2019 •

edited

448jp commented Sep 1, 2019

inouetakuya commented Sep 28, 2019

inouetakuya left a comment

inouetakuya Sep 28, 2019

inouetakuya Sep 28, 2019

inouetakuya Sep 28, 2019

ryamakuchi Sep 28, 2019

inouetakuya Sep 28, 2019

ryamakuchi Sep 28, 2019

inouetakuya left a comment •

edited

inouetakuya Sep 29, 2019

inouetakuya Sep 29, 2019

inouetakuya Sep 29, 2019

inouetakuya Sep 29, 2019

inouetakuya commented Sep 29, 2019 •

edited

inouetakuya commented Sep 29, 2019

ryamakuchi commented Sep 29, 2019

	export interface SponsorPlans {
	export interface SponsorPlan {

	expect(platinumSponsorWrapper.find(RouterLinkStub).props().to).toBe(
	expect(wrapper.find('.platinum').find(RouterLinkStub).props().to).toBe(

	import sponsorsData from '../../../fixtures/contentful/sponsorList'
	import sponsorsData from '../../../fixtures/contentful/sponsors'

feat: スポンサー一覧ページを作成する #170

feat: スポンサー一覧ページを作成する #170

Conversation

ryamakuchi commented Sep 1, 2019 • edited

TODO

レビューポイント

この PR で対応しないこと

参考

ryamakuchi commented Sep 1, 2019 • edited

メタ情報

レビュー観点

448jp commented Sep 1, 2019

inouetakuya commented Sep 28, 2019

補足

デメリット

その他の案

inouetakuya left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

結論

以下調べたログ

RFC3986

RFC2046

Choose a reason for hiding this comment

ゲッターをモックするケース

ゲッターをモックしないケース

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inouetakuya left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

inouetakuya commented Sep 29, 2019 • edited

inouetakuya commented Sep 29, 2019

ryamakuchi commented Sep 29, 2019

ryamakuchi commented Sep 1, 2019 •

edited

ryamakuchi commented Sep 1, 2019 •

edited

inouetakuya left a comment •

edited

inouetakuya commented Sep 29, 2019 •

edited