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

fix(web): resolve encryption glitch in footer text #267

Merged
merged 8 commits into from
Jun 15, 2024

Conversation

Aleesssino
Copy link
Contributor

@Aleesssino Aleesssino commented Jun 10, 2024

User description

Description

I added state management to store the element's width before the encryption animation begins. This ensures that when the encryption starts, the layout remains stable and doesn't break.

Fixes #249

Future Improvements

If others find that this fix doesn't work as expected or if the behavior isn't ideal, I will improve my solution accordingly.

Mentions

@kriptonian1 I am waiting for your feedback.

Screenshots of relevant screens

2024-06-10.19-34-32.online-video-cutter.com.mp4

Developer's checklist

  • My PR follows the style guidelines of this project
  • I have performed a self-check on my work

If changes are made in the code:

  • I have followed the coding guidelines
  • My changes in code generate no new warnings
  • My changes are breaking another fix/feature of the project
  • I have added test cases to show that my feature works
  • I have added relevant screenshots in my PR
  • There are no UI/UX issues

PR Type

Bug fix


Description

  • Added state management to store the element's width before the encryption animation begins to ensure layout stability.
  • Changed the root element from motion.button to motion.div to better handle the width styling.
  • Introduced a span with a ref to measure the text width and applied this width to the motion.div.

Changes walkthrough 📝

Relevant files
Bug fix
encrypt-text.tsx
Fix encryption animation glitch by managing element width

apps/web/src/components/ui/encrypt-text.tsx

  • Added useEffect to store the element's width before the encryption
    animation begins.
  • Changed the root element from motion.button to motion.div.
  • Added a span with ref to measure the text width.
  • Applied the measured width to the motion.div to maintain layout
    stability.
  • +19/-8   

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

    Copy link
    Contributor

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5]

    2

    🧪 Relevant tests

    No

    🔒 Security concerns

    No

    ⚡ Key issues to review

    None

    Copy link
    Contributor

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Accessibility
    Replace
    with for interactive elements to improve accessibility

    To ensure accessibility and semantic correctness, replace the

    element with a element for
    interactive elements, especially since they have event handlers like onMouseEnter and
    onMouseLeave.

    apps/web/src/components/ui/encrypt-text.tsx [58-62]

    -<motion.div
    +<motion.button
       className="group relative w-full overflow-hidden text-white/60 transition-colors hover:text-white"
       onMouseEnter={scramble}
       onMouseLeave={stopScramble}
       style={{ width: width ? `${width}px` : 'auto' }}
     >
     
    Suggestion importance[1-10]: 10

    Why: Replacing the <div> with a <button> significantly improves accessibility and semantic correctness, which is crucial for web applications. This change ensures that the interactive element is correctly recognized by assistive technologies.

    10
    Best practice
    Add a cleanup function in useEffect to reset the width state on component unmount

    Consider using a cleanup function in the useEffect hook to clear the width state when the
    component unmounts. This can help prevent potential memory leaks or unexpected behavior
    when the component is no longer in the DOM but the state updates are triggered.

    apps/web/src/components/ui/encrypt-text.tsx [20-24]

     useEffect(() => {
       if (textRef.current) {
         setWidth(textRef.current.offsetWidth)
       }
    +  return () => {
    +    setWidth(null);
    +  };
     }, [TARGET_TEXT])
     
    Suggestion importance[1-10]: 9

    Why: Adding a cleanup function in the useEffect hook is a best practice to prevent potential memory leaks and ensure that the state is reset when the component unmounts. This is a significant improvement for the stability and reliability of the component.

    9
    Performance
    Use useMemo for calculating width to optimize performance

    It is recommended to use useMemo for calculating the width based on
    textRef.current.offsetWidth to optimize performance by avoiding unnecessary recalculations
    during re-renders that do not affect the textRef.current.

    apps/web/src/components/ui/encrypt-text.tsx [20-24]

    -useEffect(() => {
    -  if (textRef.current) {
    -    setWidth(textRef.current.offsetWidth)
    -  }
    -}, [TARGET_TEXT])
    +const width = useMemo(() => {
    +  return textRef.current ? textRef.current.offsetWidth : null;
    +}, [textRef.current]);
     
    Suggestion importance[1-10]: 7

    Why: Using useMemo can optimize performance by preventing unnecessary recalculations. However, the current implementation with useEffect is also valid and the performance gain might be marginal in this specific context.

    7
    Maintainability
    Use CSS classes instead of inline styles for dynamic width adjustments

    Avoid using inline styles for setting the width dynamically. Instead, consider using CSS
    classes and modifying these classes based on the state to make the styles more
    maintainable and potentially enhance performance by reducing style recalculations.

    apps/web/src/components/ui/encrypt-text.tsx [62]

    -style={{ width: width ? `${width}px` : 'auto' }}
    +className={`group relative w-full overflow-hidden text-white/60 transition-colors hover:text-white ${width ? 'custom-width' : ''}`}
     
    Suggestion importance[1-10]: 6

    Why: While using CSS classes instead of inline styles can improve maintainability and performance, the current use of inline styles is not a critical issue. The suggestion is valid but addresses a minor improvement.

    6

    Copy link
    Contributor

    @kriptonian1 kriptonian1 left a comment

    Choose a reason for hiding this comment

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

    @Aleesssino after solving the mentioned review, we can merge it 👍

    apps/web/src/components/ui/encrypt-text.tsx Show resolved Hide resolved
    apps/web/src/components/ui/encrypt-text.tsx Outdated Show resolved Hide resolved
    Copy link
    Contributor

    @kriptonian1 kriptonian1 left a comment

    Choose a reason for hiding this comment

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

    Just add the role, and then it will be good to merge

    apps/web/src/components/ui/encrypt-text.tsx Show resolved Hide resolved
    Copy link

    sonarcloud bot commented Jun 14, 2024

    Quality Gate Passed Quality Gate passed

    Issues
    1 New issue
    0 Accepted issues

    Measures
    0 Security Hotspots
    No data about Coverage
    1.5% Duplication on New Code

    See analysis details on SonarCloud

    Copy link
    Contributor

    @kriptonian1 kriptonian1 left a comment

    Choose a reason for hiding this comment

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

    @rajdip-b LGTM, you can merge 🚀

    @rajdip-b rajdip-b merged commit d82782f into keyshade-xyz:develop Jun 15, 2024
    5 checks passed
    @kriptonian1
    Copy link
    Contributor

    @rajdip-b it doesn't seem like it's working after merged, can you confirm if it's an issue from the deployment end or not

    @rajdip-b
    Copy link
    Member

    For now, we don't have any staging environment. So, the pipeline is set to work only when code is pushed to main (when a release is made). That's why, it didn't work.

    Later on when we will completely deploy the alpha environment, we will set up develop with the staging environment. Hope that clears things out!

    @kriptonian1
    Copy link
    Contributor

    Can we merge this pr to prod

    @kriptonian1 kriptonian1 linked an issue Jun 26, 2024 that may be closed by this pull request
    rajdip-b pushed a commit that referenced this pull request Jun 27, 2024
    ## [2.1.0](v2.0.0...v2.1.0) (2024-06-27)
    
    ### 🚀 Features
    
    * **api:** Add `requireRestart` parameter ([#286](#286)) ([fb447a1](fb447a1))
    * **cli:** Added CLI ([#289](#289)) ([1143d95](1143d95))
    * **workflows:** Tag user on attempt's reply body ([9d01698](9d01698))
    
    ### 🐛 Bug Fixes
    
    * **web:** Resolve encryption glitch in footer text  ([#267](#267)) ([2b5cb39](2b5cb39))
    
    ### 📚 Documentation
    
    * added running-the-web-app.md ([#269](#269)) ([755ea12](755ea12))
    rajdip-b pushed a commit that referenced this pull request Jun 27, 2024
    ## [2.1.0](v2.0.0...v2.1.0) (2024-06-27)
    
    ### 🚀 Features
    
    * **api:** Add `requireRestart` parameter ([#286](#286)) ([fb447a1](fb447a1))
    * **cli:** Added CLI ([#289](#289)) ([1143d95](1143d95))
    * **workflows:** Tag user on attempt's reply body ([9d01698](9d01698))
    
    ### 🐛 Bug Fixes
    
    * **web:** Resolve encryption glitch in footer text  ([#267](#267)) ([2b5cb39](2b5cb39))
    
    ### 📚 Documentation
    
    * added running-the-web-app.md ([#269](#269)) ([755ea12](755ea12))
    yogesh1801 pushed a commit to yogesh1801/keyshade that referenced this pull request Jun 29, 2024
    ## [2.1.0](keyshade-xyz/keyshade@v2.0.0...v2.1.0) (2024-06-27)
    
    ### 🚀 Features
    
    * **api:** Add `requireRestart` parameter ([keyshade-xyz#286](keyshade-xyz#286)) ([fb447a1](keyshade-xyz@fb447a1))
    * **cli:** Added CLI ([keyshade-xyz#289](keyshade-xyz#289)) ([1143d95](keyshade-xyz@1143d95))
    * **workflows:** Tag user on attempt's reply body ([9d01698](keyshade-xyz@9d01698))
    
    ### 🐛 Bug Fixes
    
    * **web:** Resolve encryption glitch in footer text  ([keyshade-xyz#267](keyshade-xyz#267)) ([2b5cb39](keyshade-xyz@2b5cb39))
    
    ### 📚 Documentation
    
    * added running-the-web-app.md ([keyshade-xyz#269](keyshade-xyz#269)) ([755ea12](keyshade-xyz@755ea12))
    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.

    WEB: Fix encrypt text glitch in footer
    3 participants