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

GithubのContributionが表示されないバグを修正 #7361

Merged
merged 15 commits into from
Mar 15, 2024
Merged

GithubのContributionが表示されないバグを修正 #7361

merged 15 commits into from
Mar 15, 2024

Conversation

SuzukaHori
Copy link
Contributor

@SuzukaHori SuzukaHori commented Feb 13, 2024 •
edited
Loading

Issue

概要

ダッシュボードページで、GitHubのContributions（草）が生えないバグを修正しました。

スクリーンショット 2024-02-14 21 53 05

変更確認方法

  1. bug/fix-to-get-github-contributions-graphをローカルに取り込む

  2. foreman start -f Procfile.devでアプリを起動する

  3. 左下のSelect Userからhajimeを選んでログインする

  4. rails cでコンソールを開き、ユーザhajimeにgithub_accountを追加する

    user = User.find(253826460)
user.github_account = "GitHubユーザ名" # 例 "komagata" "SuzukaHori"など
user.save!

    ※草が生えているユーザなら誰でも問題ありません。

  5. ダッシュボード（http://localhost:3000/）を開き、GitHubの草が生えていることを確認する

  6. GitHubの草をクリックし、以下を確認する

  • GitHubのユーザページに正しくリンクしていること
  • GitHubのユーザページとダッシュボードの草が一致していること
    草が微妙に一致しない場合は、GitHubのログインの状態に起因するものなので、シークレットウィンドウ（未ログインの状態）で表示したGitHub上の草と見比べる。←2/26追記

Screenshot

変更前

スクリーンショット 2024-02-14 16 59 44

変更後

(デザイン前のため仮)
スクリーンショット 2024-02-14 17 00 02

@SuzukaHori SuzukaHori self-assigned this Feb 13, 2024
SuzukaHori
SuzukaHori commented Feb 14, 2024
View reviewed changes
app/helpers/github_grass_helper.rb
@@ -1,7 +0,0 @@
# frozen_string_literal: true
Copy link
Contributor Author

Choose a reason for hiding this comment

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

使用している箇所が見当たらなかったため、削除しました。

app/helpers/users_helper.rb
@@ -26,10 +26,6 @@ def user_submit_label(user, from)
end
end

def user_github_grass_url(user)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

使用している箇所が見当たらなかったため、削除しました。

Copy link
Member

Choose a reason for hiding this comment

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

不要なコードを積極的に削除していくのはとっても素晴らしいです〜
（追加より削除の方が難しく、どんどんコードが増えていくのがありがちなため）

app/javascript/stylesheets/application/blocks/user/_user-grass.sass Outdated
max-width: 100%
+position(absolute, top 50%)
transform: translateY(-50%)
// padding-top: 14%
Copy link
Contributor Author

Choose a reason for hiding this comment

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

この箇所があるとContributionの配置がおかしくなるため、デザインの完了まで一時的にコメントアウトしています。

Copy link
Contributor

Choose a reason for hiding this comment

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

デザインの完了はこの PR に含まれるでしょうか？

Copy link
Contributor Author

Choose a reason for hiding this comment

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

説明不足ですみません🙏

こちらのIssueはHTMLタグの取得や削除を行っていて、レビューでHTMLの中身が変わる可能性があるので、チーム開発ミーティングで確認の上、デザインより前にレビューをお願いすることにしました。

レビューでは機能のみ見ていただければ大丈夫です！

app/models/github_grass.rb Outdated
Nokogiri::HTML(html).css(SELECTOR)
def extract_table(html)
table = Nokogiri::HTML(html).css(SELECTOR)
table.css(SELECTOR_TO_REMOVE).each(&:remove)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

<table>タグを取得した際に<tool-tip>タグもついてきてしまい、ブートキャンプでは使用しないと思われるため削除しています。

<tool-tip>はこれです↓
スクリーンショット 2024-02-14 22 06 07

@SuzukaHori
Copy link
Contributor Author

SuzukaHori commented Feb 15, 2024 •
edited
Loading

レビュー・デザイン用のメモ

以下のファイルは差分が膨大ですが、以下の理由で変更しています。

  • test/cassettes/github_grass/fetch.yml
    テストの結果が記録されており、元のままだとテストを変更しても結果が反映されませんでした。
    カセットファイルを削除→テスト実行により、自動で生成されたものをコミットしています。
  • app/views/shared/_loading_grass.html.erb
    読み込み中の画面がsvgタグになっていたので、こちらもtableタグに揃えました。

@SuzukaHori
Copy link
Contributor Author

@goruchanchan
お疲れさまです！
全く急ぎではないので、お手隙の際にこちらのPRのレビューをお願いできますでしょうか🙏

お時間が取れない場合は遠慮なく仰ってください。
よろしくお願いいたします🙇‍♀️

@goruchanchan
Copy link
Contributor

@SuzukaHori レビュー依頼ありがとうございます！！一週間程度を目安に対応いたしますので、お待ちください🙇‍♂️

goruchanchan
goruchanchan reviewed Feb 19, 2024
View reviewed changes
Copy link
Contributor

@goruchanchan goruchanchan left a comment

Choose a reason for hiding this comment

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

お疲れ様です！動作確認実施しました！手順６ですが微妙に表示がずれているように見えましたが、こちらの確認方法が悪いでしょうか？🤔

image.png

また１点コメントしておりますのでお手数ですがご確認よろしくお願いします🙇‍♂️

app/javascript/stylesheets/application/blocks/user/_user-grass.sass Outdated
max-width: 100%
+position(absolute, top 50%)
transform: translateY(-50%)
// padding-top: 14%
Copy link
Contributor

Choose a reason for hiding this comment

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

デザインの完了はこの PR に含まれるでしょうか？

@SuzukaHori
Copy link
Contributor Author

@goruchanchan
お忙しい中、レビューありがとうございます🙏
コメントいただいた一点について、追加でコメントしました！
#7361 (comment)

また、草の不一致についてご指摘いただきありがとうございます🙏
こちらは原因がわからず、解決に時間がかかりそうです💦（GitHub上でデータを持ってくるときにいつの間にかdata-levelが変わっている）
調査でき次第、再度レビューをお願いしますので、少々お待ちください🙇‍♀️

@SuzukaHori
Copy link
Contributor Author

@komagata （CC:@goruchanchan）

お疲れさまです。
昨日質問しました、GitHubの草が微妙に異なる件について、原因が特定できました。
Nokogiriの問題ではありませんでした。（見当違いな方向を調べていたので、アドバイス大変助かりました🙏）

その上でご相談があるため、お手隙の際にご回答をお願いします。

原因

GitHubのプロフィールページが、ログイン状態と未ログイン状態で異なる草を表示していることだと思われます。

  • bootcamp上に作った草は、未ログイン状態の草と一致しています。

    スクショ スクリーンショット 2024-02-22 12 40 32 スクリーンショット 2024-02-22 12 40 32のコピー

  • 未ログイン状態の草は、ログイン状態の草・bootcamp上に作った草と一致していません。

    スクショ スクリーンショット 2024-02-22 12 40 32のコピー2

調べたこと

自分の草表示・goruchanchanさんのスクショ#7361 (review) 、wataさんのスクショ#6745 (review) を確認しました。

  • ログイン・未ログイン状態で、草が増える場合もあれば減る場合もある。草は9割ぐらい一致している印象。

  • 発生するのは自分の草のみ。他の人の草を見た場合は、ログイン・未ログイン状態で差はない。

  • ログイン・未ログインで草が異なる理由はよくわからなかった。

    • キャッシュの削除で変化がないので、キャッシュの問題ではなさそう。
    • GitHub Docs によると、リポジトリの閲覧権限の問題で、閲覧者と本人で草が一致しない場合があるらしい。
      →ただし以下の理由から、これが直接の原因と考えるのは疑問が残りました。
      • ログイン状態の方が草が少なくなっている状態が複数発生しており、これの説明がつかない。
      • Contributions settingsからPrivate contributionsをオンにしても草が変わらない。

聞きたいこと

  • ユーザがログインした状態の草と完全一致させる必要があるか。
    現在bootcampでは、GitHubの草は自分のみ見れる実装になっています。そのため、仮にbootcampの草とGitHubの草を見比べた場合、ほとんどがログイン状態となるため、高確率で差が発生します。

  • もしログイン済みと一致させる場合、ログイン状態でHTTPリクエストを飛ばすにはどのような方法があるか。

@komagata
Copy link
Member

@SuzukaHori

ユーザがログインした状態の草と完全一致させる必要があるか。

未ログイン状態の草で良いです。

@SuzukaHori
Copy link
Contributor Author

@komagata
回答ありがとうございます！
未ログイン状態の草を表示することにします🙏

@goruchanchan
お疲れさまです！
草の不一致の原因はログイン状態の差だと特定できました。

こちらで再度確認していただいても良いでしょうか？

確認方法：

  • GithubのContributionが表示されないバグを修正 #7361 (comment) の変更確認手順の1~5を行う。

  • GitHubのユーザページと、bootcampのダッシュボードの草が一致していることを確認する。

    • 草が一致しない場合は、未ログイン状態と比較する。（シークレットウィンドウを使うとログアウトせずに見れます）

@goruchanchan
Copy link
Contributor

goruchanchan commented Feb 23, 2024 •
edited
Loading

@SuzukaHori

ログイン状態というのは、FBCアプリへログインしたユーザが Github に対してログイン保持できている状態という理解で良いでしょうか？よって、シークレットウィンドウでアクセスすれば、未ログイン状態を再現できるということでしょうか？

上記の観点で Github に対してログイン保持していない状況では下記のように草は一致していそうなことを確認できました！

Image from Gyazo

一方で、ログインしたユーザの画面は下記になっています。（Githubのログイン状態を保持）

image.png

調べたことでコメントしているように、自身の草が上記のように見えてしまうのは、本当にやりたいことなのか微妙な気がすると感じたのですがいかがでしょうか？理解が違っていたらすみません🙇‍♂️

@SuzukaHori SuzukaHori force-pushed the bug/fix-to-get-github-contributions-graph branch from 79b7d4d to 41b3a4b Compare February 24, 2024 13:03
@SuzukaHori
Copy link
Contributor Author

@goruchanchan

ご確認ありがとうございます！

ログイン状態というのは、FBCアプリへログインしたユーザが Github に対してログイン保持できている状態という理解で良いでしょうか？よって、シークレットウィンドウでアクセスすれば、未ログイン状態を再現できるということでしょうか？

こちらの認識で間違いありません。説明が不足しており、申し訳ありません🙇‍♀️

Githubのログイン状態を保持した状態の画面について

ご指摘いただき、ありがとうございます🙏

複数のブラウザで確認しましたが、私の環境でこの状態の画面を確認できませんでした😖
スクショを貼っていただいたお陰で原因らしき箇所がわかり、修正しましたので、以下に記します。

  • スクショを見る限り、GitHubのログイン・未ログインによるHTMLの差ではなく、ラベルのデザインの崩れが問題であるように見えました。
    草自体も差があるものの9割位一致しており、周囲のデザインの崩れが修正できれば、comment-1958672477で想定した許容範囲の差になると考えました。

  • 表示の崩れは、GitHub上で非表示になっているタグ（<span class="sr-only">と<caption>）が表示されていることが原因だと思われます。

  • bootcampでは<span class="sr-only">・<caption>と同等の表示が他の箇所にあり、削除しても大きな問題はないと考えました。デザイン崩れの原因のタグを、こちらのコミット：41b3a4b で取り除きました。
    （rebaseにより差分がわかりにくくなってしまっていますが、今回の変更はこのコミットのみです🙇‍♀️）

確認して欲しいこと

崩れを自分で確認できていないため確信はないのですが、原因らしきものを取り除いたので、お手数ですがもう一度確認をお願いできますでしょうか？？

変更確認手順

  • こちらの変更確認手順1~3を行う：GithubのContributionが表示されないバグを修正 #7361 (comment)

  • ユーザhajimeのGitHubアカウントをgoruchanchanさんご自身のものに登録。

    user = User.find(253826460)
user.github_account = "goruchanchan" 
user.save!

  • GitHubのログイン状態を保持したまま、ダッシュボード（http://localhost:3000/）にアクセスする。

  • ラベルの表示が崩れていないか確認する。
    想定しているのは以下のようなラベルです。
    スクリーンショット 2024-02-24 22 37 13

  • 崩れが修正できていた場合は、以下の内容を確認する。

    • ログイン状態のGitHubのユーザページの草と、bootcampのダッシュボードの草が概ね（90%ぐらい）一致していること。

@goruchanchan
Copy link
Contributor

@SuzukaHori
ご対応ありがとうございます！また、詳細な調査もありがとうございます！変更確認手順記載の項目について、内容確認できました👍LGTMと思います👍

goruchanchan
goruchanchan approved these changes Feb 25, 2024
View reviewed changes
Copy link
Contributor

@goruchanchan goruchanchan left a comment

Choose a reason for hiding this comment

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

確認しました！！

@SuzukaHori
Copy link
Contributor Author

@goruchanchan
確認ありがとうございます🙏
お忙しい中何度も見ていただき、大変助かりました！

@SuzukaHori
Copy link
Contributor Author

@komagata
お疲れ様です。
メンバーにApproveをいただいたので、レビューをよろしくお願いします🙇‍♀️

  • こちらは2/14の開発ミーティングで、「HTMLの中身が変わる可能性があるのでデザインより前にレビューをお願いする」とご相談したIssueなので、デザインがまだ入っていない状態です。

  • 差分が膨大なファイルが2つありますが、こちらに変更に理由を書きました。GithubのContributionが表示されないバグを修正 #7361 (comment)

komagata
komagata approved these changes Mar 7, 2024
View reviewed changes
Copy link
Member

@komagata komagata left a comment

Choose a reason for hiding this comment

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

コード的にはOKです〜！

app/helpers/users_helper.rb
@@ -26,10 +26,6 @@ def user_submit_label(user, from)
end
end

def user_github_grass_url(user)
Copy link
Member

Choose a reason for hiding this comment

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

不要なコードを積極的に削除していくのはとっても素晴らしいです〜
（追加より削除の方が難しく、どんどんコードが増えていくのがありがちなため）

@SuzukaHori
Copy link
Contributor Author

@komagata
確認ありがとうございます🙏
不要なコードを削除する方が難しいかつ大事なのですね😳今後は意識したいと思います！

@machida
お疲れさまです！こちら、<table>タグに置き換える際にデザインが崩れてしまったため、修正をお願いしたいです🙏

  • 草の位置がおかしくなるため、下のファイルのCSSを一時的にコメントアウトしています。
    app/javascript/stylesheets/application/blocks/user/_user-grass.sass
    必要なければ削除をお願いします🙇‍♀️
  • HTMLの構造が変わる可能性があったため、デザインより先にレビューを進めたIssueになります。
    完了後はマージをお願いしたいです。

@machida machida marked this pull request as ready for review March 15, 2024 14:30
@machida
Copy link
Member

machida commented Mar 15, 2024

@SuzukaHori お待たせしました！デザイン入れたので、テストが通ったらマージしますー

@machida machida removed their assignment Mar 15, 2024
@machida machida force-pushed the bug/fix-to-get-github-contributions-graph branch from 3261ba0 to b5d3abc Compare March 15, 2024 15:07
@machida
Copy link
Member

machida commented Mar 15, 2024

最新のmainからrebaseしました。

@machida machida merged commit 993df93 into main Mar 15, 2024
1 check passed
@machida machida deleted the bug/fix-to-get-github-contributions-graph branch March 15, 2024 15:24
@github-actions github-actions bot mentioned this pull request Mar 15, 2024
Merged
6 tasks
@SuzukaHori
Copy link
Contributor Author

@machida
デザインとマージありがとうございます！
草が見やすい形になっているのを確認しました🙏

ステージング環境で確認中に気づいたのですが、スマホなど画面幅が狭くなった時、右側に余白が入ってしまうように見えました。
スクリーンショット 2024-03-16 11 10 51

こちら修正していただくことはできますでしょうか🙇‍♀️（もし必要でしたら私の方でIssueを立てますので、お申し付けください。）

@machida
Copy link
Member

machida commented Mar 16, 2024

@SuzukaHori
草がtableになったので、幅にぴったり合わすのができなくなってしまったんですよね。無理すればできるのですが、結構複雑になりそうです(それはそれで時間があるときに挑戦してみたいと思います)。
幅は可変でなく決めうちになってるので、GitHub本家のようにスクロールを出すか、余白が出るのどちらかになってしまいます。
なぜ決めうちがというと、テーブルのセル一つ一つの高さがコードにベタ書きされてるからなんですよね。無理すれば可変にはできるのですが、そもそもが決めうちのサイズを想定したコードになっているというのとです。
なので、一旦はこのままリリースして、表示の仕方は無理はせずオリジナルのGitHubのものと同じ方向性を保つ、という方針で行きたいと思います。サイズを可変にする場合はハックが必要になり、それが複雑になる原因になったり、GitHubの更新に追従できなくなる原因になったりする可能性が高いからです。
ただ、CSSのtransformを使ってサイズの縮小、拡大をすれば比較的ハック的なコードは少なくて済みそうなので、挑戦してみたいなとは思ってます。

@SuzukaHori
Copy link
Contributor Author

SuzukaHori commented Mar 16, 2024 •
edited
Loading

@machida
ご返信ありがとうございます🙏
改めてHTMLを確認したら確かにHTMLタグの中にサイズの指定がありました🙇‍♀️
このようなコードだと、サイズの調整ができなくなるのですね、勉強になります👀

一旦このままリリースの方針も、承知しました！

ちなみになのですが、app/models/github_grass.rbでtable・span・tr・tdタグのスタイル属性を取り除けば、幅を合わせることができたりしますか？？
styleタグを取り除いて、全部の幅高さを消すと、こんな状態になります。
スクリーンショット 2024-03-16 12 50 55

もしサイズが合わない原因がこれでしたら、私がapp/models/github_grass.rbでstyleを削除していなかったのが原因なので、別のPRで対応したいと思った次第です🙇

@machida
Copy link
Member

machida commented Mar 16, 2024

@SuzukaHori
そうですねー、今思い付く実装方法がいくつかあって、スタイルを消した方がやりやすいものもあります。とはいえ、スタイルを消しても大きさを親の幅に合わせて可変にしつつ縦横の比率を一定にするのは画像なら楽なのですが、tableだと大変であることは変わらないんですよね。取り除いてほしいスタイルを洗い出したらIssueとして登録したいと思います。

@SuzukaHori
Copy link
Contributor Author

@machida
なるほど、HTML上で幅や高さが指定されていなかったとしても、tableタグだと親のサイズに合わせるのは大変なのですね💦
その辺りの難しさがよくわかっていなかったです😅町田さんのおっしゃっている「決め打ち」の意味が理解できました！

お返事いただけて大変勉強になりました、ありがとうございました🙏

This was referenced Apr 12, 2024
Merged
Closed
@machida machida mentioned this pull request Oct 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goruchanchan goruchanchan goruchanchan approved these changes

@komagata komagata komagata approved these changes

Assignees

@SuzukaHori SuzukaHori

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@SuzukaHori @goruchanchan @komagata @machida