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

Fix/scss #236

Merged
merged 3 commits into from
Oct 24, 2024
Merged

Fix/scss #236

merged 3 commits into from
Oct 24, 2024

Conversation

Pugma
Copy link
Collaborator

@Pugma Pugma commented Oct 24, 2024

User description

これで CI 系の warning は改善終了


PR Type

enhancement, configuration changes


Description

  • SCSSのプリプロセッサオプションを更新し、api: 'modern-compiler'を使用するように変更しました。
  • SCSSのインポート方法を@importから@useに変更しました。
  • ESLint設定にvue/max-attributes-per-lineルールを追加し、1行あたりの属性数を制限しました。
  • ESLintのパーサーオプションの引用符を一貫性のあるものに調整しました。

Changes walkthrough 📝

Relevant files
Enhancement
vite.config.ts
Update SCSS preprocessor options and import method             

vite.config.ts

  • Updated SCSS preprocessor options to use api: 'modern-compiler'.
  • Changed SCSS import statement from @import to @use.
  • +2/-1     
    Configuration changes
    eslint.config.js
    Add ESLint rule for max attributes per line                           

    eslint.config.js

  • Added vue/max-attributes-per-line rule to ESLint configuration.
  • Adjusted parser option quotes for consistency.
  • +14/-2   

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    Copy link

    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Configuration Change
    The SCSS compiler API has been updated to 'modern-compiler' and the import method changed from '@import' to '@use'. Ensure compatibility and correctness of these changes.

    Rule Addition
    New ESLint rules for Vue components have been added, including custom event name casing and max attributes per line. Verify that these rules do not conflict with existing code standards.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    SCSSファイルの正確な参照を確保するためにファイル拡張子を追加する。


    additionalDataの値に使用されている@use@importに比べてモジュール性が高いですが、commonファイルの拡張子が.scssが抜けています。正しく参照するためには拡張子を含める必要があります。

    vite.config.ts [27]

    -additionalData: `@use "${srcPath}/styles/common" as *;`
    +additionalData: `@use "${srcPath}/styles/common.scss" as *;`
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a potential bug where the SCSS file extension is missing, which could lead to incorrect file resolution. Adding the extension ensures the file is referenced accurately, which is crucial for the build process.

    9
    Enhancement
    属性の最大数のルールをより実用的な値に調整する。


    vue/max-attributes-per-lineルールのsinglelinemultilinemax値が1に設定されていますが、これは非常に厳格です。より実用的な値に調整することを検討してください。

    eslint.config.js [35-43]

     'vue/max-attributes-per-line': [
       'warn',
       {
         singleline: {
    -      max: 1
    +      max: 3
         },
         multiline: {
    -      max: 1
    +      max: 2
         }
       }
     ]
    Suggestion importance[1-10]: 6

    Why: The suggestion proposes adjusting the strictness of the vue/max-attributes-per-line rule, which could improve code readability and maintainability. While subjective, the change could be beneficial for teams preferring less strict attribute limits.

    6

    Copy link
    Contributor

    @nokhnaton nokhnaton left a comment

    Choose a reason for hiding this comment

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

    vue/max-attributes-per-lineを別で設定しないといけないのはよくわかんない(flat/recommendedに含まれてるって書いてある)けど、別で書かないと動いてなさそうなのでこれで良さそう。
    https://eslint.vuejs.org/rules/max-attributes-per-line

    @Pugma Pugma enabled auto-merge October 24, 2024 17:12
    @Pugma Pugma merged commit 53a7264 into main Oct 24, 2024
    9 checks passed
    @Pugma Pugma deleted the fix/scss branch October 24, 2024 17:13
    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