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-1779] fix darkmode ui #119

Merged
merged 1 commit into from
Jun 10, 2024
Merged

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Jun 7, 2024

PR Type

Bug fix


Description

  • Adjusted the initialization order in App.vue to ensure the theme is set before initializing the app store.
  • Fixed dark mode text color issues in SignTransaction.vue by adding appropriate dark mode classes.
  • Fixed dark mode text color issues in DetailsTransactionSlideover.vue by adding appropriate dark mode classes.

Changes walkthrough 📝

Relevant files
Bug fix
App.vue
Adjust initialization order for theme and app store           

resources/js/components/App.vue

  • Moved initialTheme() call before appStore.init()
+1/-1     
SignTransaction.vue
Fix dark mode text color in transaction signing dialog     

resources/js/components/SignTransaction.vue

  • Added dark mode text color classes to transaction signing dialog
+1/-1     
DetailsTransactionSlideover.vue
Fix dark mode text color in transaction details slideover

resources/js/components/slideovers/common/DetailsTransactionSlideover.vue

  • Added dark mode text color classes to transaction details slideover
  • +1/-1     

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

    Copy link

    github-actions bot commented Jun 7, 2024

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are straightforward and localized to specific UI components, involving simple logic adjustments and CSS class additions.

    🧪 Relevant tests

    No

    ⚡ Possible issues

    Possible Race Condition: Changing the order of initialTheme() and appStore.init() in App.vue might introduce a race condition if appStore.init() relies on theme settings.

    🔒 Security concerns

    No

    Code feedback:
    relevant fileresources/js/components/App.vue
    suggestion      

    Consider verifying if appStore.init() depends on the theme settings initialized by initialTheme(). If so, ensure that any necessary theme information is available before initializing the app store. [important]

    relevant lineawait appStore.init();

    relevant fileresources/js/components/SignTransaction.vue
    suggestion      

    Ensure that the added text color classes (text-light-content-strong dark:text-dark-content-strong) are consistently applied across all relevant elements within the dialog to maintain uniformity in dark mode appearance. [medium]

    relevant line

    relevant fileresources/js/components/slideovers/common/DetailsTransactionSlideover.vue
    suggestion      

    Verify the contrast and readability of the new text color classes in different themes to ensure accessibility standards are met. [medium]

    relevant lineclass="flex h-full flex-col divide-y divide-light-stroke dark:divide-dark-stroke bg-light-surface-primary dark:bg-dark-surface-primary shadow-xl text-light-content-strong dark:text-dark-content-strong"

    Copy link

    github-actions bot commented Jun 7, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Change the order of function calls to ensure proper initialization sequence

    Consider moving the initialTheme() function call after the appStore.init() to ensure that
    the theme initialization happens after the app store has been fully initialized. This
    change might prevent potential issues where the theme relies on configurations or states
    set during the app store initialization.

    resources/js/components/App.vue [43-44]

    +await appStore.init();
     initialTheme();
    -await appStore.init();
     
    Suggestion importance[1-10]: 9

    Why: This suggestion addresses a potential issue where the theme initialization might depend on the app store's state. Ensuring the app store is initialized first can prevent bugs related to uninitialized states.

    9
    Performance
    Add a conditional rendering to apply styles only when needed

    Add a conditional rendering check for signing in the newly added div to ensure that the
    styles are applied only when necessary. This can improve performance by avoiding
    unnecessary style recalculations and re-rendering when signing is false.

    resources/js/components/SignTransaction.vue [14]

    -<div class="flex flex-col space-y-2 mt-4 relative text-light-content-strong dark:text-dark-content-strong">
    +<div v-if="signing" class="flex flex-col space-y-2 mt-4 relative text-light-content-strong dark:text-dark-content-strong">
     
    Suggestion importance[1-10]: 8

    Why: Adding a conditional rendering check for signing in the newly added div can improve performance by avoiding unnecessary style recalculations and re-rendering. This is a good practice for optimizing the component's rendering behavior.

    8
    Enhancement
    Dynamically apply the shadow-xl class based on expansion state

    Consider adding a conditional class binding to dynamically apply the shadow-xl class based
    on a condition, such as whether the transaction details are expanded or not. This can
    enhance the UI by providing visual feedback to the user.

    resources/js/components/slideovers/common/DetailsTransactionSlideover.vue [3]

    -class="flex h-full flex-col divide-y divide-light-stroke dark:divide-dark-stroke bg-light-surface-primary dark:bg-dark-surface-primary shadow-xl text-light-content-strong dark:text-dark-content-strong"
    +:class="{'flex h-full flex-col divide-y divide-light-stroke dark:divide-dark-stroke bg-light-surface-primary dark:bg-dark-surface-primary shadow-xl text-light-content-strong dark:text-dark-content-strong': true, 'shadow-xl': isExpanded}"
     
    Suggestion importance[1-10]: 7

    Why: This suggestion enhances the UI by providing visual feedback based on the expansion state. While it is a good enhancement, it is not critical to the functionality of the component.

    7

    @zlayine zlayine merged commit 3a438ef into master Jun 10, 2024
    3 checks passed
    @zlayine zlayine deleted the bugifx/pla-1779/darkmode-fixes branch June 10, 2024 04:16
    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