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

AtCoderのロゴ表示に対応 #194

Merged
merged 9 commits into from
Jul 6, 2024
Merged

AtCoderのロゴ表示に対応 #194

merged 9 commits into from
Jul 6, 2024

Conversation

Pugma
Copy link
Collaborator

@Pugma Pugma commented Jul 6, 2024

User description

#153 への部分的対応
AtCoderロゴのsvgがなかったためpngで対応


PR Type

enhancement, documentation


Description

  • AtCoderのロゴを表示するための条件分岐をAIcon.vueに追加
  • AtCoder.pngをインポートし、条件に応じて表示
  • PNGファイルの型定義をshims-png.d.tsに追加

Changes walkthrough 📝

Relevant files
Enhancement
AIcon.vue
AtCoderロゴ表示のための条件分岐と画像追加                                                                 

src/components/UI/AIcon.vue

  • AtCoderのロゴを表示するための条件分岐を追加
  • AtCoder.pngをインポート
  • v-ifを使用してimgタグを条件付きでレンダリング
  • +3/-0     
    Documentation
    shims-png.d.ts
    PNGファイルの型定義追加                                                                                       

    src/types/shims-png.d.ts

    • PNGモジュールの型定義を追加
    +5/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    github-actions bot commented Jul 6, 2024

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    条件分岐のロジック:
    name !== 'atcoder' の条件でAtCoderのロゴを表示するかどうかを判断していますが、この条件が正しいか確認してください。意図したすべてのケースで正しく動作するか、テストを行うことをお勧めします。

    Copy link

    github-actions bot commented Jul 6, 2024

    PR Code Suggestions ✨

    Latest suggestions up to c155c46

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    v-if 条件式を修正して、'atcoder' の場合に AtCoder のロゴが表示されるようにします。

    v-if 条件式が間違っている可能性があります。条件式が services.get(account.type)?.icon !== 'atcoder'
    となっていますが、これではアイコンが 'atcoder' でない場合に AtCoder のロゴが表示されることになります。正しくは 'atcoder' の場合に AtCoder
    のロゴを表示するべきです。

    src/components/User/AccountListItem.vue [22-28]

     <img
    -  v-if="services.get(account.type)?.icon !== 'atcoder'"
    +  v-if="services.get(account.type)?.icon === 'atcoder'"
       alt="AtCoder Logo"
       :src="AtCoder"
       width="24px"
       height="24px"
     />
     
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a logical error in the v-if condition, ensuring that the AtCoder logo is displayed only when the icon type is 'atcoder'. This is a significant fix to ensure the correct behavior of the application.

    9
    Enhancement
    alt 属性の値を修正して、画像の内容を正確に説明します。

    alt 属性の値が "AtCoder Logo" となっていますが、これは誤りです。この画像はユーザーのアカウントタイプを表すアイコンであるため、適切な説明は "User
    Account Icon" などが適切です。

    src/components/User/AccountListItem.vue [24]

     <img
       v-if="services.get(account.type)?.icon === 'atcoder'"
    -  alt="AtCoder Logo"
    +  alt="User Account Icon"
       :src="AtCoder"
       width="24px"
       height="24px"
     />
     
    Suggestion importance[1-10]: 6

    Why: The suggestion improves the accessibility of the image by providing a more accurate description in the alt attribute. While this is a good enhancement for accessibility, it is not as critical as fixing a logical error.

    6

    Previous suggestions

    Suggestions up to commit 1e4253f
    CategorySuggestion                                                                                                                                    Score
    Accessibility
    タグに alt 属性を追加する。

    <img> タグに alt 属性を追加して、画像が表示されない場合やスクリーンリーダーを使用しているユーザーに対応してください。

    src/components/UI/AIcon.vue [28]

    -<img v-else :src="AtCoder" :style="styles" />
    +<img v-else :src="AtCoder" :style="styles" alt="AtCoder Logo" />
     
    Suggestion importance[1-10]: 9

    Why: Adding an alt attribute to the <img> tag significantly improves accessibility, making the application more inclusive for users with disabilities.

    9
    Maintainability
    画像のインポートパスを相対パスに変更する。

    AtCoder
    画像のインポートパスが絶対パスで指定されていますが、これを相対パスに変更してください。これにより、プロジェクトの構造が変更された場合でも、パスの変更が容易になります。

    src/components/UI/AIcon.vue [3]

    -import AtCoder from '/@/assets/AtCoder.png'
    +import AtCoder from '@/assets/AtCoder.png'
     
    Suggestion importance[1-10]: 8

    Why: Changing the import path to a relative path enhances maintainability, making the codebase more robust to structural changes.

    8
    条件式を逆にして直感的にする。

    v-if 条件を name === 'atcoder' に変更して、条件を逆にしてください。これにより、条件がより直感的になり、読みやすくなります。

    src/components/UI/AIcon.vue [22]

    -v-if="name !== 'atcoder'"
    +v-if="name === 'atcoder'"
     
    Suggestion importance[1-10]: 7

    Why: This suggestion improves code readability by making the condition more intuitive. However, it is a minor change and does not affect functionality.

    7
    Best practice
    アイコン名の管理を computed プロパティで行う。

    タグの :data-icon 属性に直接 name をバインドするのではなく、computed プロパティを使用して、アイコン名の変更があった場合に対応できるようにしてください。

    src/components/UI/AIcon.vue [24]

    -:data-icon="name"
    +:data-icon="iconName"
     
    Suggestion importance[1-10]: 6

    Why: Using a computed property for the icon name can improve code maintainability and readability, but it is not crucial for the current functionality.

    6

    @Pugma
    Copy link
    Collaborator Author

    Pugma commented Jul 6, 2024

    /improve

    Copy link
    Contributor

    @mehm8128 mehm8128 left a comment

    Choose a reason for hiding this comment

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

    consts/services.ts
    icon: 'atcoder' //アイコンは保留のコメントを消しておいてほしいです🙏

    src/components/User/AccountListItem.vue Show resolved Hide resolved
    @Pugma Pugma requested a review from mehm8128 July 6, 2024 04:55
    @Pugma Pugma requested a review from mehm8128 July 6, 2024 06:39
    @Pugma Pugma requested a review from mehm8128 July 6, 2024 06:54
    Copy link
    Contributor

    @mehm8128 mehm8128 left a comment

    Choose a reason for hiding this comment

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

    :tasukatta::kansya_kansya:

    @Pugma Pugma merged commit b9a149d into master Jul 6, 2024
    8 checks passed
    @Pugma Pugma deleted the feat/atcoderLogo branch July 6, 2024 06:57
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    2 participants