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

サイドバーがヘッダーを覆うように修正 #401

Merged
merged 4 commits into from
Dec 3, 2024

Conversation

ErrorSyntax1
Copy link
Contributor

@ErrorSyntax1 ErrorSyntax1 commented Dec 3, 2024

User description

close #297


PR Type

Enhancement


Description

  • サイドバーがヘッダーを完全に覆うように修正しました。これにより、top: 5rem のスタイルを削除し、サイドバーの表示が改善されました。
  • ナビゲーションバーに閉じるボタンを追加しました。アイコンとラベルを含む新しいデザインで、ユーザーが簡単にサイドバーを閉じることができます。
  • ナビゲーションリンクのレイアウトを柔軟にするために、.listflex-grow: 1 を追加しました。

Changes walkthrough 📝

Relevant files
Enhancement
App.vue
Adjust sidebar to fully cover the header                                 

src/App.vue

  • Removed the top: 5rem style from .navigationBarCover and
    .navigationBar to ensure the sidebar fully covers the header.
  • Added v-model binding to navigation-bar for better two-way data
    binding.
  • +1/-2     
    NavigationBar.vue
    Add close button and update navigation bar layout               

    src/components/NavigationBar/NavigationBar.vue

  • Added a close button with an icon and label to the navigation bar.
  • Introduced a new closeContainer style for the close button layout.
  • Adjusted the container layout by adding a gap and removing
    justify-content: space-between.
  • +23/-1   
    NavigationLinks.vue
    Improve navigation links layout flexibility                           

    src/components/NavigationBar/NavigationLinks.vue

    • Added flex-grow: 1 to .list for better layout flexibility.
    +1/-0     

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

    @ErrorSyntax1 ErrorSyntax1 requested a review from Pugma December 3, 2024 05:08
    Copy link
    Contributor

    github-actions bot commented Dec 3, 2024

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    297 - Partially compliant

    Fully compliant requirements:

    • サイドバーがヘッダーとの間に隙間がないように修正する

    Not compliant requirements:

    • サイドバーの上部にロゴを配置する
    ⏱️ Estimated effort to review: 3 🔵🔵🔵⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Code Consistency
    v-modelの使用が一貫していない可能性があります。isOpenNavigationBarの状態管理方法を確認してください。

    Syntax Error
    defineModelは正しい関数名ではありません。definePropsを使用する必要があります。

    Copy link
    Contributor

    github-actions bot commented Dec 3, 2024

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    General
    defineModelの代わりにdefinePropsを使用してプロパティを定義します。

    defineModelの代わりにdefinePropsを使用して、Vue 3のComposition
    APIでプロパティを定義することをお勧めします。これにより、より一般的で理解しやすいコードになります。

    src/components/NavigationBar/NavigationBar.vue [2-7]

    -import {defineModel} from 'vue'
    +import {defineProps} from 'vue'
     ...
    -const a = defineModel<boolean>({required: true})
    +const a = defineProps<{ required: boolean }>()
    Suggestion importance[1-10]: 8

    Why: Using defineProps instead of defineModel aligns better with the standard practices of Vue 3 Composition API, enhancing code readability and maintainability. This suggestion correctly identifies a more appropriate API usage for defining component props.

    8
    v-modelを使用する代わりにイベントハンドラを使用して状態を明示的に管理します。


    v-modelを使用する場合、対応するデータプロパティが更新されるときに意図しない副作用が発生する可能性があるため、v-modelの代わりにイベントハンドラを使用して明示的に状態を管理することを検討してください。

    src/App.vue [39]

     <navigation-bar
       v-if="isOpenNavigationBar"
    -  v-model="isOpenNavigationBar"
    +  :is-open="isOpenNavigationBar"
    +  @update:is-open="isOpenNavigationBar = $event"
       :class="$style.navigationBar"
     />
    Suggestion importance[1-10]: 7

    Why: The suggestion to replace v-model with an explicit event handler can help in controlling the component state more predictably, which is particularly useful in complex applications to avoid unintended side effects.

    7

    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.

    ここだけ確認お願いします!

    import NavigationLinks from './NavigationLinks.vue'
    import NavigationBarFooter from './NavigationBarFooter.vue'
    import Icon from '../UI/Icon.vue'

    const a = defineModel<boolean>({ required: true })
    Copy link
    Collaborator

    Choose a reason for hiding this comment

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

    こんな感じで修正しておいてください!

    Suggested change
    const a = defineModel<boolean>({ required: true })
    const isSidebarOpen = defineModel<boolean>({ required: true })

    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.

    改めてになってすみません!
    これもお願いします!

    それ以外は良さそうです

    src/components/NavigationBar/NavigationBar.vue Outdated Show resolved Hide resolved
    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.

    動作確認もできました!
    ありがとう!良さそうです

    @ErrorSyntax1 ErrorSyntax1 merged commit d12632e into main Dec 3, 2024
    10 checks passed
    @ErrorSyntax1 ErrorSyntax1 deleted the fix/sidebar_design branch December 3, 2024 05:27
    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.

    sp: サイドバーを開いたときにヘッダーとの間に隙間がある
    2 participants