Skip to content

Conversation

@ArshdeepGrover
Copy link
Owner

@ArshdeepGrover ArshdeepGrover commented Mar 2, 2025

PR Type

Enhancement, Tests


Description

  • Added a new Resume section with routing.

  • Introduced HomePageComponent and refactored existing layout.

  • Updated navigation to include a link to the Resume section.

  • Added unit tests for HomePageComponent and ResumeComponent.


Changes walkthrough 📝

Relevant files
Enhancement
10 files
app-routing.module.ts
Added routing for `HomePageComponent` and `ResumeComponent`
+14/-3   
app.component.ts
Removed fragment-based scrolling logic                                     
+1/-20   
app.module.ts
Declared `HomePageComponent` and `ResumeComponent`             
+4/-0     
home-page.component.ts
Implemented `HomePageComponent` with fragment scrolling   
+32/-0   
resume.component.ts
Implemented `ResumeComponent` to display PDF                         
+15/-0   
app.component.html
Replaced static layout with `router-outlet`                           
+1/-40   
header.component.html
Updated CV link to route to `ResumeComponent`                       
+1/-4     
home-page.component.html
Added HTML structure for `HomePageComponent`                         
+40/-0   
resume.component.html
Added HTML to embed resume PDF                                                     
+8/-0     
home-page.component.scss
Added styles for `HomePageComponent`                                         
+13/-0   
Tests
2 files
home-page.component.spec.ts
Added unit tests for `HomePageComponent`                                 
+28/-0   
resume.component.spec.ts
Added unit tests for `ResumeComponent`                                     
+28/-0   
Additional files
2 files
.nojekyll [link]   
resume.component.scss [link]   

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @vercel
    Copy link

    vercel bot commented Mar 2, 2025

    The latest updates on your projects. Learn more about Vercel for Git ↗︎

    Name Status Preview Comments Updated (UTC)
    arshdeep-singh-grover ❌ Failed (Inspect) Apr 24, 2025 6:35am

    @qodo-code-review
    Copy link

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 PR contains tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Navigation Issue

    The CV link opens in a new tab (target="_blank") while being a router link. This may cause unexpected behavior since router navigation is meant for internal navigation within the same window.

    <a [routerLink]="['./resume']" target="_blank">
    PDF Loading

    The PDF embed assumes the resume file exists at a specific path (/assets/resume/arshdeep-resume.pdf) without any error handling if the file is not found or fails to load.

    src="/assets/resume/arshdeep-resume.pdf#toolbar=0"

    @qodo-code-review
    Copy link

    qodo-code-review bot commented Mar 2, 2025

    Qodo Merge was enabled for this repository. To continue using it, please link your Git account with your Qodo account here.

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add PDF loading error handling

    Add error handling for cases where the PDF file fails to load. Consider showing
    a fallback message or alternative content when the PDF cannot be displayed.

    src/app/component/resume/resume.component.html [2-8]

     <div style="height: 100dvh">
       <embed
         src="/assets/resume/arshdeep-resume.pdf#toolbar=0"
         style="width: 100%; height: 100%;"
         type="application/pdf"
    +    onerror="this.style.display='none'; this.nextElementSibling.style.display='block';"
       />
    +  <div style="display: none; text-align: center; padding: 20px;">
    +    Unable to load PDF. Please try again later or contact support.
    +  </div>
     </div>
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    __

    Why: The suggestion adds important error handling for PDF loading failures, which improves user experience by providing clear feedback when the resume cannot be displayed. This is a critical enhancement for reliability.

    Medium
    Remove unnecessary target attribute
    Suggestion Impact:The commit addressed the issue but with a different approach. Instead of simply removing the target='_blank' attribute, the developer changed the implementation completely by replacing the routerLink with a direct href to a PDF file, where target='_blank' is appropriate.

    code diff:

    -      <a [routerLink]="['./resume']" target="_blank">
    +      <a
    +        href="https://arshdeep-singh.vercel.app//assets/resume/arshdeep-singh.pdf"
    +        target="_blank"
    +        title="Arshdeep's Resume"
    +      >

    Remove target="_blank" from the router link since it's unnecessary for internal
    navigation and can cause unexpected behavior.

    src/app/component/header/header.component.html [20]

    -<a [routerLink]="['./resume']" target="_blank">
    +<a [routerLink]="['./resume']">

    [Suggestion has been applied]

    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies that using target="_blank" with routerLink is inappropriate for internal navigation and could lead to navigation issues. Removing it helps maintain proper single-page application behavior.

    Medium
    • Update

    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.

    1 participant