update 'no-errors' page design & content #610
Conversation
WalkthroughThe changes in this pull request primarily involve updates to the HTML template and associated test files. The HTML file now includes a conditional block for setting the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Coverage Report
File CoverageNo changed files found. |
rosado
force-pushed
the
rosado/571-content-and-styling-1
branch
from
48f1572 to
60afcff
Compare
rosado
force-pushed
the
rosado/571-content-and-styling-1
branch
from
98a0306 to
a1b23fd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
test/PageObjectModels/resultsPage.js (1)
19-19: Consider making the regex pattern more specific.The current pattern
/You have \d+ row.* ready to publish/works but could be more precise. A more specific pattern would better document the expected format and catch potential issues.Consider using this pattern instead:
- expect(await this.page.locator('h1').innerText()).toMatch(/You have \d+ row.* ready to publish/) + expect(await this.page.locator('h1').innerText()).toMatch(/You have \d+ row(?:s)? ready to publish/)src/views/check/results/no-errors.html (1)
48-48: Consider adding ARIA attributes for screen readers.While the dynamic heading is informative, consider adding ARIA attributes to enhance accessibility when the count changes.
- You have {{options.pagination.totalResults}} {{ "row" | pluralise(options.pagination.totalResults) }} ready to publish + You have <span aria-live="polite">{{options.pagination.totalResults}} {{ "row" | pluralise(options.pagination.totalResults) }}</span> ready to publishtest/integration/test_recieving_results.playwright.test.js (1)
Line range hint
1-1: Fix the typo in the filename.The filename contains a spelling error: "recieving" should be "receiving".
Apply this change:
-test/integration/test_recieving_results.playwright.test.js +test/integration/test_receiving_results.playwright.test.js
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
src/views/check/results/no-errors.html(3 hunks)test/PageObjectModels/resultsPage.js(1 hunks)test/acceptance/request_check.test.js(0 hunks)test/integration/test_recieving_results.playwright.test.js(1 hunks)test/integration/validation_errors.playwright.test.js(0 hunks)
💤 Files with no reviewable changes (2)
- test/acceptance/request_check.test.js
- test/integration/validation_errors.playwright.test.js
🔇 Additional comments (3)
test/PageObjectModels/resultsPage.js (1)
19-19: Consider adding test cases for different row counts.
The method now handles dynamic content, but it would be beneficial to verify it works correctly with different scenarios.
Let's check if there are any existing test cases:
Would you like me to help create additional test cases to verify the behaviour with:
- Single row (singular form)
- Multiple rows (plural form)
- Large number of rows
src/views/check/results/no-errors.html (1)
20-24: LGTM! Verify deep link generation.
The conditional logic for setting checkToolLink is well-structured with a proper fallback.
Let's verify the deep link generation implementation:
✅ Verification successful
Deep link implementation is properly tested and secure
The checkToolDeepLink filter is well-implemented with:
- Proper URL encoding of parameters to prevent injection
- Clear type definitions and documentation
- Comprehensive test coverage for both valid and invalid scenarios
- Correct registration in the filters module
The conditional usage in no-errors.html aligns perfectly with the implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the checkToolDeepLink filter implementation
# Expected: Find the filter implementation to ensure it handles all edge cases
# Search for the checkToolDeepLink filter implementation
rg -l "checkToolDeepLink"
# Search for tests covering the deep link generation
rg -l "test.*checkToolDeepLink|describe.*checkToolDeepLink"
Length of output: 377
Script:
#!/bin/bash
# Let's examine the implementation and test coverage
# Check the filter implementation
cat src/filters/checkToolDeepLink.js
# Check the test implementation
cat test/unit/deepLinkController.test.js
# Check how it's registered in filters
cat src/filters/filters.js
Length of output: 4065
test/integration/test_recieving_results.playwright.test.js (1)
27-27: LGTM! The regex pattern accommodates dynamic row counts.
The updated assertion using a regular expression pattern /You have \d+ row.* ready to publish/ is more flexible and aligns well with the dynamic content changes in the 'no-errors' page.
GeorgeGoodall-GovUk
temporarily deployed
to
development
GitHub Actions
Inactive
What type of PR is this? (check all applicable)
Description
Update design and content of the 'no-errors' template, and a deep link into the check tool.
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Top of the page now looks like this:
Bottom of the page looks like this:
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
[optional] Are there any post deployment tasks we need to perform?
[optional] Are there any dependencies on other PRs or Work?
Summary by CodeRabbit
New Features
Bug Fixes
Tests