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.

Sign up for GitHub

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

Jump to bottom

提出物タブでもマイプロフィール確認と日報作成がしたい #5760

Merged
merged 2 commits into from
Nov 21, 2022
Merged

提出物タブでもマイプロフィール確認と日報作成がしたい #5760

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
06a9a73
提出物タブでもマイプロフィール確認と日報作成がしたい
ksmxxxxxx Nov 11, 2022
b9884c4
マイプロフィールのパスをシンプルに変更
ksmxxxxxx Nov 19, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions app/views/current_user/products/index.html.slim
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,17 @@ header.page-header
.page-header__inner
h2.page-header__title
| ダッシュボード
.page-header-actions
.page-header-actions__items
Copy link
Member

Choose a reason for hiding this comment

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

9行目の.page-header-actions__itemsにはulタグを付与するのが正しいみたいです！

[192] スクラム#3 | FBCこちらの日報でmachidaさんが仰っていました！

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OdenTakashi
レビューありがとうございます。

こちら確かに町田さんのおっしゃるとおりで、ulとliでマークアップするのが良さそうというのは同意です。
更に言うとナビゲーションに該当すると思うので、.page-header-actionsにnavでマークアップするのが良いのでは？という気になりポイントがあります。

ただ、対応することにしたときに、一点気になったことがあります。
ここ以外にも同様のナビゲーションがダッシュボードのトップや日報タブにも実装されています。同時進行で出している #5758 にもあります。
もしここを ul,liにするのだとしたら、ここだけマークアップされているのも不揃になるので合わせてマークアップを修正するのが妥当だと考えますが、今回のIssueの範囲からはみ出てしまうので、このPRで対応するべきか？という疑問が出てきます。

自分の経験則だと、基本「Issueからはみ出る範囲のものは別Issueを作成してそちらで対応する」というのが筋の良いスプリントの進め方かなぁと考えています。（コミットもまとめられるので履歴もきれいになりそう）

提案としては、

  1. このPRではあくまで実装するにとどめて、マークアップの修正については別のIssueで対応することを開発MTGでチームリーダー（この場合komagataさんになると思います）に提案する
  2. このPRだけはマークアップの修正を行って、他のタブの同様のナビゲーションは別Issueを作成してそこで対応する
  3. どうするのがいいのかwakaranので、水曜日の開発MTGで共有してPO（町田さんになるのかな）に判断を仰ぐ

の3つになります。
OdenTakashiさんはどうおもいますか？
自分は1がいいかなぁって考えてます（多分、現場でもそう判断すると思います）。

Copy link
Member

Choose a reason for hiding this comment

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

@ksmxxxxxx
ご連絡遅れてしまい申し訳ありません🙏

なるほどです！確かに今回のみマークアップの修正を行ってしまうと不揃いになってしまいますね💦
machidaさんもマークアップができていない箇所を見つけ次第issueを立て欲しいとおっしゃっていたので、マークアップの修正については別issueを立てて対応するという方が良いのかなと感じたので、私も１がいいかなと考えました！

このような視点を考えずコメントをしてしまっていました💦
申し訳ありません🙇‍♂️

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@OdenTakashi こちら開発MTGでmachidaさんに確認させてもらいました。方針については別途まとめてコメントに残しておきたいと思います〜

.page-header-actions__item
Copy link
Member

Choose a reason for hiding this comment

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

10行目のpage-header-actions__itemにはliタグを付与するのが正しいみたいです！

= link_to user_path(id: current_user.id), class: 'a-button is-md is-secondary is-block' do
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
= link_to user_path(id: current_user.id), class: 'a-button is-md is-secondary is-block' do
= link_to current_user, class: 'a-button is-md is-secondary is-block' do

こちらでいいかもです〜

Copy link
Contributor Author

Choose a reason for hiding this comment

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

b9884c4
修正しました〜

i.fa-solid.fa-user
| マイプロフィール
- unless current_user.adviser?
.page-header-actions__item
Copy link
Member

Choose a reason for hiding this comment

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

15行目のpage-header-actions__itemにはliタグを付与するのが正しいみたいです！

= link_to new_report_path, class: 'a-button is-md is-secondary is-block' do
i.fa-regular.fa-plus
| 日報作成

= render 'home/page_tabs', user: @user

Expand Down