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

iconify周りをupdate #264

Merged
merged 7 commits into from
Jul 17, 2024
Merged

iconify周りをupdate #264

merged 7 commits into from
Jul 17, 2024

Conversation

mehm8128
Copy link
Contributor

@mehm8128 mehm8128 commented May 12, 2024

User description

close #210

issueのリンク先にVue用のものがあったのでそれを使用
多分これで問題ないはず
https://iconify.design/docs/icon-components/vue/


PR Type

enhancement, dependencies


Description

  • FormInputコンポーネントのアイコン表示ロジックを改善し、props.iconのundefinedチェックを簡略化しました。
  • Iconコンポーネントをリファクタリングし、@iconify/vueIconコンポーネントを使用するように変更しました。
  • 不要な@purge-icons/generatedインポートを削除しました。
  • vite.config.tsからPurgeIconsプラグインを削除しました。
  • package.jsonpackage-lock.jsonの依存関係を更新し、@iconify/iconifyと関連パッケージを削除し、@iconify/vueを追加しました。

Changes walkthrough 📝

Relevant files
Enhancement
FormInput.vue
`FormInput`コンポーネントのアイコン表示ロジックを改善                                                 

src/components/UI/FormInput.vue

  • props.iconのundefinedチェックを簡略化
  • iconコンポーネントのicon属性を削除
+2/-2     
Icon.vue
`Icon`コンポーネントのリファクタリングと`@iconify/vue`の使用                                 

src/components/UI/Icon.vue

  • @iconify/vueからIconをインポート
  • stylesの計算を削除
  • iconコンポーネントの使用に変更
  • +3/-13   
    Dependencies
    main.ts
    不要な`@purge-icons/generated`インポートの削除                                           

    src/main.ts

    • @purge-icons/generatedのインポートを削除
    +0/-1     
    vite.config.ts
    `PurgeIcons`プラグインの削除                                                                         

    vite.config.ts

    • PurgeIconsプラグインの削除
    +1/-2     
    package-lock.json
    パッケージの依存関係の更新                                                                                       

    package-lock.json

  • @iconify/iconifyと関連パッケージの削除
  • @iconify/vueの追加
  • vite-plugin-purge-iconsと関連パッケージの削除
  • +26/-177
    package.json
    パッケージの依存関係の更新                                                                                       

    package.json

  • @iconify/iconifyと関連パッケージの削除
  • @iconify/vueの追加
  • vite-plugin-purge-iconsの削除
  • +2/-3     

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

    @mehm8128 mehm8128 requested a review from Pugma May 12, 2024 14:33
    @mehm8128 mehm8128 self-assigned this May 12, 2024
    @Pugma Pugma marked this pull request as draft July 16, 2024 16:34
    @Pugma Pugma marked this pull request as ready for review July 16, 2024 16:35
    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Code Simplification
    props.iconのundefinedチェックが簡略化されましたが、新しいチェック方法が正確に機能するか確認が必要です。

    Refactoring
    Iconコンポーネントが@iconify/vueに完全に依存するように変更されました。新しい実装が既存の機能に影響を与えないか検証が必要です。

    Dependency Changes
    @iconify/vueが追加され、古いIconify関連パッケージが削除されました。これによる影響を確認し、必要な依存関係が正しく更新されているかを検証してください。

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    props.icon の存在チェックを改善します。

    props.icon のチェックを undefined ではなく、真偽値チェックに変更してください。これにより、空文字や null が渡された場合にも正しく動作します。

    src/components/UI/FormInput.vue [36]

    -<div v-if="props.icon !== undefined" :class="$style.iconContainer">
    +<div v-if="props.icon" :class="$style.iconContainer">
     
    Suggestion importance[1-10]: 9

    Why: This suggestion improves the robustness of the code by ensuring that props.icon is truthy, which covers more cases such as empty strings or null values, enhancing the reliability of the component.

    9
    @iconify/vue のバージョンを最新に更新する。

    @iconify/vue バージョンを最新の安定版に更新してください。現在のバージョン 4.1.2
    は古い可能性があります。最新の安定版を使用することで、セキュリティのリスクを減らし、新しい機能を利用できます。

    package.json [28]

    -"@iconify/vue": "^4.1.2",
    +"@iconify/vue": "^4.1.3",  # 仮の最新バージョン
     
    Suggestion importance[1-10]: 7

    Why: Updating to the latest stable version can reduce security risks and provide new features, but the suggested version 4.1.3 is hypothetical and should be verified.

    7
    @iconify/vue の導入に伴い、アイコン管理方法を見直し、必要に応じてリファクタリングを行う。

    @iconify/vue
    の追加により、アイコン関連の機能が強化される可能性がありますが、既存のアイコン管理方法との整合性を確認し、必要に応じてリファクタリングを行うことをお勧めします。

    package.json [28]

    -"@iconify/vue": "^4.1.2",
    +"@iconify/vue": "^4.1.2",  # リファクタリング後も必要であれば保持
     
    Suggestion importance[1-10]: 5

    Why: Ensuring consistency with existing icon management is important, but the suggestion to add a comment is minor and does not directly improve the code.

    5
    Possible bug
    アイコン名の参照方法を修正します。

    コンポーネントの :icon 属性を :icon="mdi:${name}" に変更して、アイコン名の前に mdi:
    を追加してください。これにより、アイコンの参照が正しく行われます。

    src/components/UI/Icon.vue [12]

    -<icon :icon="name" :height="size" :width="size" :class="$style.icon" />
    +<icon :icon="`mdi:${name}`" :height="size" :width="size" :class="$style.icon" />
     
    Suggestion importance[1-10]: 8

    Why: This suggestion ensures that the icon name is correctly prefixed with mdi:, which is necessary for proper icon referencing. This addresses a potential bug in icon rendering.

    8
    Best practice
    @openapitools/openapi-generator-cli のバージョン指定をより安定した形式に変更する。

    @openapitools/openapi-generator-cli のバージョンが ^2.7.0
    と指定されていますが、このバージョン指定は将来的に非互換な更新が適用される可能性があります。より安定したバージョン指定を使用することをお勧めします。

    package.json [29]

    -"@openapitools/openapi-generator-cli": "^2.7.0",
    +"@openapitools/openapi-generator-cli": "~2.7.0",
     
    Suggestion importance[1-10]: 8

    Why: Changing the version specification to a more stable format can prevent future compatibility issues, making this a good best practice.

    8
    プロパティのデフォルト値の設定方法を改善します。

    size プロパティのデフォルト値を設定する際に、withDefaults を使用するのではなく、Props
    インターフェースで直接デフォルト値を設定してください。これにより、コードの可読性が向上します。

    src/components/UI/Icon.vue [8]

    -withDefaults(defineProps<Props>(), { size: 24 })
    +defineProps<Props>({
    +  name: String,
    +  size: { type: Number, default: 24 }
    +})
     
    Suggestion importance[1-10]: 7

    Why: While this suggestion improves code readability by setting default values directly in the Props interface, it is more of a stylistic improvement rather than a critical change.

    7
    Maintainability
    新たに追加された依存関係の必要性を見直し、プロジェクトのメンテナンス性を向上させる。

    @iconify/vue@openapitools/openapi-generator-cli
    の追加に伴い、プロジェクトの依存関係が増えています。不要な依存関係がないか確認し、プロジェクトのメンテナンス性を向上させることをお勧めします。

    package.json [28-29]

    -"@iconify/vue": "^4.1.2",
    -"@openapitools/openapi-generator-cli": "^2.7.0",
    +"@iconify/vue": "^4.1.2",  # 依存関係の見直し後も必要であれば保持
    +"@openapitools/openapi-generator-cli": "^2.7.0",  # 依存関係の見直し後も必要であれば保持
     
    Suggestion importance[1-10]: 6

    Why: Reviewing dependencies for necessity can improve maintainability, but the suggestion to add comments is not crucial and does not directly improve the code.

    6

    Copy link
    Collaborator

    @Pugma Pugma left a comment

    Choose a reason for hiding this comment

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

    レビュー遅くなってしまってすみません
    勝手にmainブランチからのマージをしちゃいました

    @iconify/vue のパラメータに対応していていいと思います!
    AtCoderロゴ追加のために多少追加で編集したので、そちらの確認が済み次第マージしていただいて大丈夫です!

    @mehm8128 mehm8128 merged commit babd2ed into main Jul 17, 2024
    9 checks passed
    @mehm8128 mehm8128 deleted the feat/update_iconify branch July 17, 2024 08:44
    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.

    iconを使うパッケージの乗り換え
    2 participants