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

feat: extend um-no-capture to check parent elements #152

Merged

Conversation

seeratawan01
Copy link
Member

@seeratawan01 seeratawan01 commented Nov 12, 2024

PR Type

enhancement


Description

  • Enhanced the shouldCaptureDomEvent function to prevent event capturing if any parent element has the um-no-capture class.
  • Improved handling of shadow DOM elements by checking the host element.
  • Refactored code for better readability and consistency, including the removal of unused variables.
  • Updated logic to identify useful parent elements based on tag names and computed styles.

Changes walkthrough 📝

Relevant files
Enhancement
autocapture-utils.ts
Extend DOM event capture logic to include parent elements

packages/javascript-sdk/src/utils/autocapture-utils.ts

  • Extended the shouldCaptureDomEvent function to check parent elements
    for the um-no-capture class.
  • Improved handling of shadow DOM elements.
  • Refactored code for clarity and consistency.
  • Enhanced logic to determine if a parent is a useful element.
  • +39/-30 

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

    @github-actions github-actions bot added the enhancement New feature or request label Nov 12, 2024
    Copy link

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🏅 Score: 85
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Redundant Code
    The logic for handling shadow DOM and checking for useful elements is duplicated, which could be refactored into a separate function to improve code maintainability and reduce redundancy.

    Code feedback:
    relevant filepackages/javascript-sdk/src/utils/autocapture-utils.ts
    suggestion      

    Consider refactoring the repeated logic for handling shadow DOM and checking parent elements into a separate function. This will make the code cleaner and more maintainable. [important]

    relevant lineif (curEl.parentNode && isDocumentFragment(curEl.parentNode)) {

    relevant filepackages/javascript-sdk/src/utils/autocapture-utils.ts
    suggestion      

    It seems that the check for 'um-no-capture' class is done in a loop for each parent element until the body tag is reached. This could potentially be optimized by breaking early from the loop once a match is found, instead of continuing to traverse up the DOM tree. [important]

    relevant lineif (curEl.classList && curEl.classList.contains('um-no-capture')) {

    relevant filepackages/javascript-sdk/src/utils/autocapture-utils.ts
    suggestion      

    To improve performance, consider caching the result of window.getComputedStyle for the current element outside the loop since it's used multiple times. This avoids redundant calls to getComputedStyle, which can be costly in terms of performance. [important]

    relevant lineconst compStyles = window.getComputedStyle(parentNode);

    @seeratawan01 seeratawan01 merged commit 787f1f9 into develop Nov 12, 2024
    1 of 2 checks passed
    Copy link

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Simplify class checking with the closest() method

    Use a more robust method for checking if an element or its parents have a specific
    class by using closest() method instead of manual traversal, which simplifies the
    code and improves performance.

    packages/javascript-sdk/src/utils/autocapture-utils.ts [109-113]

    -let curEl: Element | null = el;
    -while (curEl && !isTag(curEl, 'body')) {
    -    if (curEl.classList && curEl.classList.contains('um-no-capture')) {
    -        return false;
    -    }
    -    ...
    +if (el.closest('.um-no-capture')) {
    +    return false;
     }
    Suggestion importance[1-10]: 8

    Why: Using the closest() method simplifies the code and potentially improves performance by reducing the complexity of manual DOM traversal. This suggestion enhances the code by making it more concise and potentially faster.

    8
    Maintainability
    Refactor repeated code into a function for better maintainability

    Consider refactoring the repeated code for handling shadow DOM and setting
    parentIsUsefulElement into a separate function to improve code maintainability and
    reduce redundancy.

    packages/javascript-sdk/src/utils/autocapture-utils.ts [126-131]

    -if (curEl.parentNode && isDocumentFragment(curEl.parentNode)) {
    -    curEl = (curEl.parentNode as any).host;
    -    if (curEl && usefulElements.indexOf(curEl.tagName.toLowerCase()) > -1) {
    -        parentIsUsefulElement = true;
    -    }
    -    continue;
    -}
    +handleShadowDOMAndSetUsefulness(curEl);
    Suggestion importance[1-10]: 7

    Why: Refactoring the repeated shadow DOM handling code into a separate function would significantly improve maintainability and reduce redundancy. This is a valuable suggestion that enhances the code structure and readability.

    7
    Possible bug
    Ensure safe type casting for DOM node assignments

    Ensure that the curEl variable is properly cast when assigning from parentNode.
    TypeScript may require explicit type assertions for DOM manipulations, especially
    when dealing with potentially null values.

    packages/javascript-sdk/src/utils/autocapture-utils.ts [118]

    -curEl = curEl.parentNode as Element;
    +curEl = curEl.parentNode as Element | null;
    Suggestion importance[1-10]: 2

    Why: The suggestion to add '| null' to the type assertion is unnecessary because TypeScript's type system handles this implicitly when the 'strictNullChecks' option is enabled. The suggestion does not significantly improve the code's robustness or safety.

    2

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    enhancement New feature or request
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    1 participant