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

Fix/button link #353

Merged
merged 7 commits into from
Sep 20, 2024
Merged

Fix/button link #353

merged 7 commits into from
Sep 20, 2024

Conversation

ogu-kazemiya
Copy link
Contributor

@ogu-kazemiya ogu-kazemiya commented Aug 23, 2024 •
edited by github-actions bot
Loading

User description

BaseButtonにto属性をつけたときに、元のBaseButtonと同じ見た目のrouter-linkになるようにした
router-linkとBaseButtonが入れ子になっているところをそれに合わせてすべて修正

close #338

PR Type

enhancement

Description

  • BaseButtonコンポーネントにto属性を追加し、router-linkを統合しました。
  • 各ページでrouter-linkを削除し、BaseButtonを使用してナビゲーションを簡素化しました。
  • 不要なCSSクラス.linkを削除しました。

Changes walkthrough 📝

Relevant files
Enhancement
17 files
ContestTeams.vue
Replace `router-link` with `BaseButton` for navigation     

src/components/Contest/ContestTeams.vue

  • Removed router-link and used BaseButton with to attribute.
  • Simplified button structure for navigation.
  • Removed unnecessary CSS class .link.
    		•  +6/-13   
    BaseButton.vue
    Integrate routing capability into `BaseButton`                     

    src/components/UI/BaseButton.vue

  • Added to prop to BaseButton for navigation.
  • Used router-link conditionally based on to prop.
  • Moved .link CSS styling to BaseButton.
    		•  +20/-1   
    UserProfileDesktop.vue
    Use `BaseButton` for navigation in UserProfileDesktop       

    src/components/User/UserProfileDesktop.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +3/-8     
    UserProfileMobile.vue
    Use `BaseButton` for navigation in UserProfileMobile         

    src/components/User/UserProfileMobile.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +3/-8     
    Contest.vue
    Simplify navigation using `BaseButton` in Contest page     

    src/pages/Contest.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +12/-18 
    ContestEdit.vue
    Use `BaseButton` for navigation in ContestEdit page           

    src/pages/ContestEdit.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +6/-15   
    ContestNew.vue
    Use `BaseButton` for navigation in ContestNew page             

    src/pages/ContestNew.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +8/-14   
    ContestTeamEdit.vue
    Use `BaseButton` for navigation in ContestTeamEdit page   

    src/pages/ContestTeamEdit.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +5/-9     
    ContestTeamNew.vue
    Use `BaseButton` for navigation in ContestTeamNew page     

    src/pages/ContestTeamNew.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +5/-9     
    Contests.vue
    Use `BaseButton` for navigation in Contests page                 

    src/pages/Contests.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +6/-9     
    Event.vue
    Use `BaseButton` for navigation in Event page                       

    src/pages/Event.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +8/-14   
    Project.vue
    Use `BaseButton` for navigation in Project page                   

    src/pages/Project.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +8/-14   
    ProjectNew.vue
    Use `BaseButton` for navigation in ProjectNew page             

    src/pages/ProjectNew.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +8/-14   
    Projects.vue
    Use `BaseButton` for navigation in Projects page                 

    src/pages/Projects.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +6/-13   
    UserAccountEdit.vue
    Use `BaseButton` for navigation in UserAccountEdit page   

    src/pages/UserAccountEdit.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +8/-14   
    UserAccountNew.vue
    Use `BaseButton` for navigation in UserAccountNew page     

    src/pages/UserAccountNew.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +8/-14   
    UserAccounts.vue
    Use `BaseButton` for navigation in UserAccounts page         

    src/pages/UserAccounts.vue

  • Replaced router-link with BaseButton for navigation.
  • Removed unnecessary CSS class .link.
    		•  +13/-17 

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

    @github-actions GitHub Actions
    Copy link
    Contributor

    PR Reviewer Guide 🔍

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

    @github-actions GitHub Actions
    Copy link
    Contributor

    github-actions bot commented Aug 23, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    icon コンポーネントをインポートする。

    コンポーネント内で icon プロパティを使用していますが、icon コンポーネントのインポートが見当たりません。icon
    コンポーネントをインポートする必要があります。

    src/components/Contest/ContestTeams.vue [32-36]

    +<script>
+import Icon from '/@/components/UI/Icon.vue'
+</script>
 <base-button
   :to="{ name: 'ContestTeamNew', params: { contestId: contestId } }"
   type="primary"
   icon="mdi:plus"
   >New</base-button
 >
    Suggestion importance[1-10]: 8

    Why: The suggestion correctly identifies the need to import the Icon component, which is used within the <base-button> component. This is a necessary enhancement to ensure the icon functionality works as intended.

    		8
    ボタンの display スタイルを flex に変更する。

    コンポーネントのスタイルで display: inline-flex;
    を使用していますが、これによりボタンのレイアウトが意図しない形で変更される可能性があります。必要に応じてスタイルを調整してください。

    src/components/UI/BaseButton.vue [47-51]

     .button {
-  display: inline-flex;
+  display: flex;
   align-items: center;
   justify-content: center;
   gap: 4px;
   padding: 8px 24px;
 }
    Suggestion importance[1-10]: 5

    Why: Changing display: inline-flex; to display: flex; might affect the layout of the button. However, without further context on the intended design, this suggestion is more of a stylistic choice rather than a necessary improvement.

    		5
    Performance
    props.to が未定義の場合に <router-link> をレンダリングしないようにする。

    コンポーネントの props.to が未定義の場合に
    がレンダリングされないように、条件を追加することをお勧めします。これにより、不要なマークアップを防ぎ、パフォーマンスを向上させることができます。

    src/components/UI/BaseButton.vue [24-32]

    -<router-link
-  v-if="props.to"
-  :to="props.to"
-  :class="[$style.button, $style.link]"
-  :data-button-type="props.type"
->
-  <icon v-if="props.icon" :name="props.icon" />
-  <slot />
-</router-link>
+<template v-if="props.to">
+  <router-link
+    :to="props.to"
+    :class="[$style.button, $style.link]"
+    :data-button-type="props.type"
+  >
+    <icon v-if="props.icon" :name="props.icon" />
+    <slot />
+  </router-link>
+</template>
    Suggestion importance[1-10]: 7

    Why: The suggestion to wrap <router-link> in a <template> with a v-if condition is redundant since v-if is already applied directly to <router-link>. However, it does emphasize the importance of avoiding unnecessary rendering, which is a valid performance consideration.

    		7

    @Pugma
    Copy link
    Collaborator

    Pugma commented Aug 23, 2024

    これは僕個人の意見なんだけど、一つのファイルにまとめてしまうよりも、BaseLink.vue というファイルを新しく作って完全に機能を分離したほうが分かりやすいかもな〜って思いました

    今だとリンクとして置きたくても、例えば :to 属性を設定し忘れば自動でボタンになっちゃいます
    こういうときに、リンク用でファイルを分けて、リンクの方には :to 属性を必須にしておけばミスも減るんじゃないかな〜って思いました

    @ogu-kazemiya
    Copy link
    Contributor Author

    なるほど
    どっちにするか迷ってたんですが、分けたほうが良さそうですね

    通常のリンクと区別したいので、コンポーネント名はLinkButtonにします

    mehm8128
    mehm8128 approved these changes Sep 19, 2024
    View reviewed changes
    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.

    よさそうです！:kansya:

    @ogu-kazemiya ogu-kazemiya merged commit 69e9f58 into main Sep 20, 2024
    9 checks passed
    @ogu-kazemiya ogu-kazemiya deleted the fix/button-link branch September 20, 2024 07:18
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @mehm8128 mehm8128 mehm8128 approved these changes

    @Pugma Pugma Awaiting requested review from Pugma

    Assignees
    No one assigned
    Labels
    Review effort [1-5]: 3
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.

    ボタンの当たり判定がバグってる
    3 participants
    @ogu-kazemiya @Pugma @mehm8128