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-1906] add wallet accounts #137

Merged
merged 1 commit into from
Jul 23, 2024
Merged

Conversation

zlayine
Copy link
Contributor

@zlayine zlayine commented Jul 22, 2024

PR Type

Enhancement, Bug fix


Description

  • Enhanced the wallet account display in SettingsWalletAccount.vue by adding labels for wallet accounts with copy-to-clipboard functionality.
  • Updated the layout to display connected wallets and introduced a copyText method to handle clipboard copying.
  • Modified walletAccount and userWalletAccounts computations to support multi-tenant configurations.
  • Added the walletAccounts field to the User GraphQL query in User.ts.
  • Fixed a minor issue by removing a redundant space in the class binding in FormInput.vue.

Changes walkthrough 📝

Relevant files
Bug fix
FormInput.vue
Remove redundant space in class binding                                   

resources/js/components/FormInput.vue

  • Removed redundant space in class binding.
+1/-1     
Enhancement
SettingsWalletAccount.vue
Enhance wallet account display and add copy functionality

resources/js/components/pages/SettingsWalletAccount.vue

  • Added labels for wallet accounts with copy-to-clipboard functionality.
  • Updated layout to display connected wallets.
  • Introduced copyText method to handle clipboard copying.
  • Modified walletAccount and userWalletAccounts computations.
  • +31/-7   
    User.ts
    Add walletAccounts field to User GraphQL query                     

    resources/js/graphql/query/auth/User.ts

    • Added walletAccounts field to the User GraphQL query.
    +1/-0     

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

    Copy link

    PR Reviewer Guide 🔍

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

    Accessibility Concern
    The labels for wallet accounts are marked as 'disabled' but are clickable and trigger the 'copyText' function. This could be misleading for accessibility tools and users. Consider using a more appropriate tag or adjusting the accessibility roles and properties.

    Code Duplication
    The 'copyText' function is used in multiple places with similar UI elements. Consider refactoring to use a component or a more centralized method to handle repetitive UI logic.

    Copy link

    github-actions bot commented Jul 22, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Accessibility
    Add 'aria-disabled' attribute to disabled labels for better accessibility

    Ensure accessibility by adding 'aria-disabled' attribute to disabled labels to
    improve screen reader support.

    resources/js/components/pages/SettingsWalletAccount.vue [17-23]

     <label
         v-if="walletAccount"
         class="text-light-content dark:text-dark-content bg-light-surface-background dark:bg-dark-surface-background text-sm rounded-md mr-auto p-2 cursor-pointer"
         name="walletAccount"
         disabled
    +    aria-disabled="true"
         @click="copyText(walletAccount)"
     >
     
    Suggestion importance[1-10]: 9

    Why: Adding the 'aria-disabled' attribute significantly improves accessibility for screen reader users, making it an important enhancement.

    9
    Maintainability
    Use a more descriptive function and parameter names for clarity

    Consider using a more descriptive variable name than 'text' to enhance code
    readability and maintainability.

    resources/js/components/pages/SettingsWalletAccount.vue [56-58]

    -const copyText = (text: string) => {
    -    navigator.clipboard.writeText(text);
    +const copyToClipboard = (clipboardContent: string) => {
    +    navigator.clipboard.writeText(clipboardContent);
         snackbar.info({ title: 'Copied to clipboard!' });
     };
     
    Suggestion importance[1-10]: 8

    Why: Using more descriptive names for the function and parameter improves code readability and maintainability, which is beneficial for future development and debugging.

    8
    Enhancement
    Add hover effects for disabled state to improve UI consistency

    Consider adding a hover effect for the disabled state to enhance UI consistency and
    user experience.

    resources/js/components/FormInput.vue [40]

    -'block flex-1 flex-shrink-0 border-0 bg-light-surface-background dark:bg-dark-surface-background text-light-content-strong dark:text-dark-content-strong ring-1 ring-inset ring-light-stroke-strong dark:ring-dark-stroke-strong  placeholder:text-light-content placeholder:dark:text-dark-content focus:ring-2 focus:ring-inset focus:ring-primary text-sm leading-6 overflow-hidden transition-all disabled:bg-dark-surface-background/10 disabled:dark:bg-light-surface-background/10 outline-none'
    +'block flex-1 flex-shrink-0 border-0 bg-light-surface-background dark:bg-dark-surface-background text-light-content-strong dark:text-dark-content-strong ring-1 ring-inset ring-light-stroke-strong dark:ring-dark-stroke-strong  placeholder:text-light-content placeholder:dark:text-dark-content focus:ring-2 focus:ring-inset focus:ring-primary text-sm leading-6 overflow-hidden transition-all disabled:bg-dark-surface-background/10 disabled:dark:bg-light-surface-background/10 disabled:hover:bg-dark-surface-background/20 disabled:dark:hover:bg-light-surface-background/20 outline-none'
     
    Suggestion importance[1-10]: 7

    Why: Adding a hover effect for the disabled state can improve UI consistency and user experience. However, it is a minor enhancement and not crucial for functionality.

    7

    @zlayine zlayine merged commit 171390e into master Jul 23, 2024
    3 checks passed
    @zlayine zlayine deleted the feature/pla-1906/wallet-accounts-ui branch July 23, 2024 07:01
    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