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

router-linkのto属性でオブジェクトの使用・HyperText型のurl(string型)をpath(RouteLocationNamedRaw)に #336

Merged
merged 6 commits into from
Aug 19, 2024

Conversation

dye8128
Copy link
Contributor

@dye8128 dye8128 commented Jul 17, 2024

User description

close #314
#334 の続きです、対応完了しました


PR Type

enhancement, bug fix


Description

  • path プロパティの型を string から RouteLocationNamedRaw に変更
  • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
  • useParam 関数で undefined の場合にデフォルト値を設定
  • isActive 計算プロパティのロジックを修正

Changes walkthrough 📝

Relevant files
Enhancement
22 files
PageInfoPanel.vue
`path` プロパティの型変更                                                                                 

src/components/Index/PageInfoPanel.vue

  • path プロパティの型を string から RouteLocationNamedRaw に変更
+2/-1     
PageInfoPanels.vue
`path` プロパティの値をオブジェクト形式に変更                                                             

src/components/Index/PageInfoPanels.vue

  • path プロパティの値をオブジェクト形式に変更
+1/-1     
ContentHeader.vue
`url` プロパティを `path` に変更                                                                   

src/components/Layout/ContentHeader.vue

  • url プロパティを path に変更し、型を RouteLocationNamedRaw に変更
+3/-2     
NavigationLinksItem.vue
`path` プロパティの型変更と `isActive` ロジック修正                                           

src/components/NavigationBar/NavigationLinksItem.vue

  • path プロパティの型を string から RouteLocationNamedRaw に変更
  • isActive 計算プロパティのロジックを修正
  • +3/-7     
    ProjectItem.vue
    `router-link` の `to` 属性をオブジェクト形式に変更                                           

    src/components/Projects/ProjectItem.vue

    • router-linkto 属性をオブジェクト形式に変更
    +4/-1     
    Contest.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/Contest.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +5/-2     
    ContestEdit.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/ContestEdit.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +9/-3     
    ContestNew.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/ContestNew.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +2/-2     
    ContestTeamEdit.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/ContestTeamEdit.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +14/-5   
    ContestTeamNew.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/ContestTeamNew.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +10/-4   
    Contests.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/Contests.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +1/-1     
    Event.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/Event.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +5/-2     
    Events.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/Events.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +1/-1     
    Index.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/Index.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +1/-1     
    Project.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/Project.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +9/-3     
    ProjectNew.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/ProjectNew.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +2/-2     
    Projects.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/Projects.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +1/-1     
    User.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/User.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +1/-1     
    UserAccountEdit.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/UserAccountEdit.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +3/-3     
    UserAccountNew.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/UserAccountNew.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +3/-3     
    UserAccounts.vue
    `header-texts` の `url` プロパティを `path` に変更                                 

    src/pages/UserAccounts.vue

    • header-textsurl プロパティを path に変更し、オブジェクト形式に変更
    +2/-2     
    routeInfo.ts
    `path` プロパティの型変更とデフォルト値修正                                                               

    src/lib/routeInfo.ts

  • path プロパティの型を string から RouteLocationNamedRaw に変更
  • デフォルト値の path を空オブジェクトに変更
  • +7/-6     
    Bug fix
    1 files
    param.ts
    `useParam` 関数のデフォルト値設定                                                                     

    src/lib/param.ts

    • useParam 関数で undefined の場合にデフォルト値を設定
    +1/-1     

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

    UserAccountEditの一か所だけ保留
    UserAccountEditのリンク訂正
    routeinfoの型変更
    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    Default Value Handling
    useParam関数でundefinedを文字列'undefined'に変換して返すのは不適切かもしれません。通常、undefinednullは明示的に扱うべきです。デフォルト値として空文字列やnullを返す方が適切かもしれません。

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible bug
    isActiveの計算プロパティを修正して、新しいprops.pathオブジェクトに対応させる。


    isActiveの計算プロパティが、props.pathの変更に対応していません。props.pathがオブジェクトに変更されたため、currentRoute.nameと比較するのではなく、currentRoute.nameprops.path.nameを比較する必要があります。

    src/components/NavigationBar/NavigationLinksItem.vue [19]

    -return currentRoute.name === props.name
    +return currentRoute.name === props.path.name
     
    Suggestion importance[1-10]: 9

    Why: The suggestion correctly identifies a potential bug where the isActive computed property should compare currentRoute.name with props.path.name instead of props.name. This change is crucial for the correct functionality of the component.

    9
    Possible issue
    useParam関数の戻り値を修正して、undefinedを文字列ではなくそのまま返すようにする。


    useParam関数でtoStringIfArray関数の戻り値がundefinedの場合に'undefined'文字列を返すようにしていますが、これは意図した動作ではない可能性があります。undefinedをそのまま返すべきです。

    src/lib/param.ts [13]

    -return computed(() => toStringIfArray(route.params[paramName]) ?? 'undefined')
    +return computed(() => toStringIfArray(route.params[paramName]))
     
    Suggestion importance[1-10]: 8

    Why: The suggestion addresses a potential issue where returning the string 'undefined' could lead to unexpected behavior. Returning undefined directly is more appropriate and aligns with typical JavaScript practices.

    8
    Enhancement
    Propsインターフェースのpathプロパティの型をより汎用的なものに変更する。


    Propsインターフェースのpathプロパティの型をRouteLocationNamedRawからRouteLocationRawに変更することを検討してください。これにより、名前付きルートだけでなく、パスベースのルートも許可されるようになります。

    src/components/Index/PageInfoPanel.vue [23]

    -path: RouteLocationNamedRaw
    +path: RouteLocationRaw
     
    Suggestion importance[1-10]: 7

    Why: Changing the type from RouteLocationNamedRaw to RouteLocationRaw would make the path property more flexible, allowing for both named and path-based routes. This is a good enhancement but not critical.

    7
    HeaderText型のpathプロパティをオプショナルにして柔軟性を向上させる。


    HeaderText型のpathプロパティをオプショナルにすることを検討してください。すべてのヘッダーテキストがルートに関連付けられているわけではないため、この変更により柔軟性が向上します。

    src/components/Layout/ContentHeader.vue [7]

    -path: RouteLocationNamedRaw
    +path?: RouteLocationNamedRaw
     
    Suggestion importance[1-10]: 6

    Why: Making the path property optional in the HeaderText type increases flexibility, allowing for header texts that are not necessarily linked to a route. This is a useful enhancement but not essential.

    6

    currentRoute.path === props.path ||
    currentRoute.path.startsWith(`${props.path}/`)
    )
    return currentRoute.name === props.name
    Copy link
    Member

    Choose a reason for hiding this comment

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

    ロジックが変わっちゃってそうです

    1. /にアクセスしてるときにTopリンクがactiveにならなくなってしまってそうです
    2. コンテストの詳細など、コンテスト一覧より下のページを見ているときにContestリンクがactiveにならなくなってしまってそうです(プロフィールやプロジェクトも同様)

    Comment on lines 19 to 29
    const findPath = (name: string): string => {
    for (const route of routes) {
    if (route.name === name) {
    return route.path
    }
    }
    return 'Undefined'
    }

    const isActive = computed(() => {
    if (props.path === '/') return currentRoute.path === props.path
    const path = findPath(String(props.path.name))
    Copy link
    Member

    Choose a reason for hiding this comment

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

    こんな感じにfindPathがもっと簡潔にできそうです

    const isActive = computed(() => {
    	const path = routes.find(route => route.name === props.path.name)?.path
    	if (path === undefined) {
    	  return false
    	}
    	
    	return (
    	...

    currentRoute.path === props.path ||
    currentRoute.path.startsWith(`${props.path}/`)
    currentRoute.name === props.path.name ||
    currentRoute.path.startsWith(`${path}/`)
    Copy link
    Member

    Choose a reason for hiding this comment

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

    これでも多分動きはするのですが、pathundefinedだったときにstartsWith('undefined/')になっちゃってあんまりきれいじゃないので僕が書いたようにundefinedのときは明示的にfalseを返すようにした方がよさそうです(テンプレートリテラル内にstring型以外を入れられないようにするeslintのルールも確かあったはずです)

    Copy link
    Member

    @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.

    ↑以外は問題なさそうなので、直したらマージしちゃってOKです

    @dye8128 dye8128 merged commit 017c5c5 into main Aug 19, 2024
    9 checks passed
    @dye8128 dye8128 deleted the fix/url-type branch August 19, 2024 02:01
    @dye8128
    Copy link
    Contributor Author

    dye8128 commented Aug 19, 2024

    確認抜けでした、ごめんなさい...!
    訂正&マージしました!

    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.

    router-linkのパラメータを第二引数で指定するようにする
    2 participants