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

[PLA-1973] fix banner #178

Merged
merged 1 commit into from
Nov 22, 2024
Merged

[PLA-1973] fix banner #178

merged 1 commit into from
Nov 22, 2024

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Nov 22, 2024

PR Type

bug_fix


Description

  • Fixed the logic for determining banner visibility by checking local storage for the banner key.
  • Ensured that the banner is not shown if the key is set to 'true' in local storage.

Changes walkthrough 📝

Relevant files
Bug fix
DynamicBanner.vue
Fix banner visibility logic using local storage                   

resources/js/components/DynamicBanner.vue

  • Modified the enabled computed property logic.
  • Changed condition to check local storage for banner key.
  • +1/-1     

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

    @zlayine zlayine self-assigned this Nov 22, 2024
    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

    Code Clarity
    The logic for checking local storage in the computed property might not account for null or undefined values which could lead to unexpected behavior.

    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add a check to ensure banner.value.key is defined before accessing localStorage

    Ensure that the localStorage.getItem(banner.value.key) call is safely handled in
    case banner.value.key is undefined or null, which could throw an error or behave
    unexpectedly.

    resources/js/components/DynamicBanner.vue [38]

    -return banner.value && localStorage.getItem(banner.value.key) !== 'true';
    +return banner.value && banner.value.key && localStorage.getItem(banner.value.key) !== 'true';
    Suggestion importance[1-10]: 8

    Why: This suggestion correctly identifies a potential bug where banner.value.key might be undefined, leading to errors when accessing localStorage. Adding a check enhances the robustness of the code by preventing runtime errors.

    8

    @zlayine zlayine merged commit cf52172 into master Nov 22, 2024
    4 checks passed
    @zlayine zlayine deleted the hotfix/pla-1973/banner branch November 22, 2024 10:59
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Development

    Successfully merging this pull request may close these issues.

    2 participants