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

タグ一覧画面を追加 #288

Merged
merged 5 commits into from Oct 10, 2019

Conversation

MitarashiDango
Copy link
Contributor

どのようなタグが使われているかを一覧化し、気になるタグからオカズへ到達しやすくするため、タグ一覧画面を作成しました。

仕様

  • 公開チェックインに紐付いているタグを1ページあたり100件ずつ表示しています
  • ソート順は1.チェックイン数の降順、2.タグ名の昇順の順番です
  • 最大4列表示
  • タグ名が長い場合、後半を一部省略して表示 (マウスオーバーで全量表示可)

課題

  • ボタンの色どうしよう(今は検索の「関連するタグ」に色を合わせてピンクにしている)
  • どこかで使いまわしたりするならタグボタンをcomponentsに切り出した方がいい?
  • タグ画面はログイン必須にする?(お総菜コーナーはログイン必須だが検索は非ログイン状態でも可能であったため、いったん検索側の仕様に倒してます)

@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #288 into develop will decrease coverage by 0.53%.
The diff coverage is 0%.

Impacted file tree graph

@@              Coverage Diff              @@
##             develop     #288      +/-   ##
=============================================
- Coverage      35.84%   35.31%   -0.54%     
- Complexity       296      300       +4     
=============================================
  Files             60       61       +1     
  Lines           1127     1144      +17     
=============================================
  Hits             404      404              
- Misses           723      740      +17
Impacted Files Coverage Δ Complexity Δ
app/Http/Controllers/TagController.php 0% <0%> (ø) 4 <4> (?)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2454a24...900e4c9. Read the comment docs.

@shibafu528
Copy link
Member

🙇‍♂️ 要変更点

  • 非ログイン時に、年齢確認中に効かせているブラーがタグボタンに適用されていません。現行の tissue.css では .container にしか効かせていないのが原因で、修正が必要です。

🤔 検討

  • 非公開アカウントのチェックインは数に入れないほうが良いのでは?

💬 課題について

ボタンの色

私はこれでいいと思います。

タグボタンをcomponentsに切り出した方がいい?

本画面と関連タグ検索画面くらいでしか使わなそうな気はしますね……。

タグを探すための機能という点で類似しているので、見た目を一貫させるべきかの議論をしたほうが良いのかも?

タグ画面はログイン必須にする?

検索画面がゲスト許可なの忘れてた。でも検索機能に準拠するのが妥当だろうと思いますので、現状のゲスト許可でOKです。

))
->join('ejaculation_tag', 'tags.id', '=', 'ejaculation_tag.tag_id')
->join('ejaculations', 'ejaculations.id', '=', 'ejaculation_tag.ejaculation_id')
->where('ejaculations.is_private', false)
Copy link
Member

Choose a reason for hiding this comment

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

グループ化+カウントの部分は

$tags = Tag::select(['tags.name'])
    ->withCount(['ejaculations as checkins_count' => function ($query) {
        $query->where('is_private', false);
    }])

にしたほうがコードはスッキリしそうです。実行SQLはきしょいですけど

ref: https://readouble.com/laravel/5.5/ja/eloquent-relationships.html#counting-related-models

@MitarashiDango
Copy link
Contributor Author

先ほど気づきましたが、タグ一覧の取得処理はチェックイン実行ユーザーが鍵垢である場合を考慮する必要がありそうですね。。

@MitarashiDango
Copy link
Contributor Author

タグ一覧のチェックイン件数カウント条件を検索機能の表示条件と揃える対応を実施しました。

(withCountの件については、joinせずにチェックイン0件のタグを一覧から除外する方法が思いつかなかったので一旦保留)

@shibafu528 shibafu528 self-requested a review September 28, 2019 08:52
Copy link
Member

@shibafu528 shibafu528 left a comment

Choose a reason for hiding this comment

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

要変更点および検討事項について、対応を確認しました。

withCountは確かに……数日以内に思いついたら書きます、思いつかなかったらとりあえずマージしようかな。

@shibafu528
Copy link
Member

思いつかないからマージします。

@shibafu528 shibafu528 merged commit 4ca6f00 into shikorism:develop Oct 10, 2019
@shibafu528 shibafu528 mentioned this pull request Dec 12, 2019
5 tasks
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

2 participants