search

nash1111 / nash1111-tech-blog

0 stars 0 forks source link

feat: add smartphone screenshot #93

Closed nash1111 closed 3 days ago

nash1111 commented 3 days ago

User description

Why

Closes #92

What

PR Type

Enhancement, Tests

Description

Changes walkthrough 📝

Relevant files
Enhancement
lastUpdated.ts
Update lastUpdated timestamp                                                         
public/lastUpdated.ts - Updated the `lastUpdated` timestamp.
+1/-1     
currentIssues.json
Update current issues list                                                             
public/currentIssues.json - Updated the list of current issues to include a new issue.
+1/-1     
Tests
task.spec.tsx
Add Playwright tests for page screenshots                               
tests/task.spec.tsx
  • Modified test to take a screenshot of the task page.
  • Added a new test to take screenshots of the blog page, including a
    smartphone-sized screenshot.
    		•  +11/-2   

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

    cloudflare-pages[bot] commented 3 days ago

    Deploying nash1111-tech-blog with  Cloudflare Pages  Cloudflare Pages

    Latest commit: d7f9fda
    Status: ✅  Deploy successful!
    Preview URL: https://1a6318bc.nash1111-tech-blog.pages.dev
    Branch Preview URL: https://issue-92.nash1111-tech-blog.pages.dev

    View logs

    cloudflare-pages[bot] commented 3 days ago

    Deploying nash1111-tech-blog with  Cloudflare Pages  Cloudflare Pages

    Latest commit: 63bac7c
    Status:⚡️  Build in progress...

    View logs

    github-actions[bot] commented 3 days ago

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review [1-5] 2
    🧪 Relevant tests Yes
    🔒 Security concerns No
    ⚡ Key issues to review Possible Bug:
    The screenshots taken in tests/task.spec.tsx do not have a specified format (e.g., PNG, JPEG). It's recommended to specify the format to ensure consistent output across different environments.
    Code Quality:
    The lastUpdated.ts file does not have a newline at the end of the file. It's a common practice to end files with a newline to comply with POSIX standards.
    github-actions[bot] commented 3 days ago

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Ensure elements are present before taking screenshots to capture fully loaded pages ___ **It's a good practice to check for the presence of elements before taking screenshots to
    ensure the page has loaded correctly. This can prevent screenshots of incomplete or
    loading pages.** [tests/task.spec.tsx [5-6]](https://github.com/nash1111/nash1111-tech-blog/pull/93/files#diff-267b23c47783cce9f2ec506ab35728afd899f7b64c0a8a39ee429ca5fd2af106R5-R6) ```diff await page.goto('http://localhost:5173/task'); +await page.waitForSelector('#expected-element'); // Adjust selector as needed await page.screenshot({ path: `test-results/screenshots/task-page.png` }); ```
    Suggestion importance[1-10]: 10 Why: This suggestion addresses a critical aspect of visual testing by ensuring that screenshots are taken only after the page has fully loaded, preventing incomplete captures.
    		10
    Set the viewport size before navigating to the page for consistent screenshot results ___ **Consider setting the viewport size before navigating to the page to ensure the screenshot
    captures the intended layout immediately. This avoids potential issues where the layout
    might change after navigation but before the screenshot is taken.** [tests/task.spec.tsx [12-15]](https://github.com/nash1111/nash1111-tech-blog/pull/93/files#diff-267b23c47783cce9f2ec506ab35728afd899f7b64c0a8a39ee429ca5fd2af106R12-R15) ```diff +await page.setViewportSize({ width: 375, height: 667 }); // iPhone 6/7/8 dimensions await page.goto('http://localhost:5173/blog'); await page.screenshot({ path: `test-results/screenshots/blog-page.png` }); -await page.setViewportSize({ width: 375, height: 667 }); // iPhone 6/7/8 dimensions await page.screenshot({ path: `test-results/screenshots/blog-page-iphone.png` }); ```
    Suggestion importance[1-10]: 9 Why: This suggestion improves the reliability of the screenshots by ensuring the layout is consistent before navigation, which is crucial for accurate visual testing.
    		9
    Reset the viewport size after each test to ensure test isolation ___ **To ensure test isolation and avoid potential side effects, consider resetting the viewport
    size after each test or setting a default size in a global setup function.** [tests/task.spec.tsx [14]](https://github.com/nash1111/nash1111-tech-blog/pull/93/files#diff-267b23c47783cce9f2ec506ab35728afd899f7b64c0a8a39ee429ca5fd2af106R14-R14) ```diff await page.setViewportSize({ width: 375, height: 667 }); // iPhone 6/7/8 dimensions +// Reset to default after test +test.afterEach(async ({ page }) => { + await page.setViewportSize({ width: 1920, height: 1080 }); // Default dimensions +}); ```
    Suggestion importance[1-10]: 7 Why: This suggestion promotes test isolation and prevents side effects between tests, which is a good practice for reliable test results.
    		7
    Maintainability
    Use a function to generate paths for screenshots to improve maintainability ___ **To avoid hardcoding paths and improve maintainability, consider using a function or a
    constant to generate paths for screenshots. This makes it easier to change the base path
    in one place rather than in multiple test cases.** [tests/task.spec.tsx [6-15]](https://github.com/nash1111/nash1111-tech-blog/pull/93/files#diff-267b23c47783cce9f2ec506ab35728afd899f7b64c0a8a39ee429ca5fd2af106R6-R15) ```diff -await page.screenshot({ path: `test-results/screenshots/task-page.png` }); -await page.screenshot({ path: `test-results/screenshots/blog-page.png` }); -await page.screenshot({ path: `test-results/screenshots/blog-page-iphone.png` }); +const screenshotPath = (filename) => `test-results/screenshots/${filename}`; +await page.screenshot({ path: screenshotPath('task-page.png') }); +await page.screenshot({ path: screenshotPath('blog-page.png') }); +await page.screenshot({ path: screenshotPath('blog-page-iphone.png') }); ```
    Suggestion importance[1-10]: 8 Why: This suggestion enhances maintainability by centralizing the path generation, making it easier to update paths in the future.
    		8