Remove numeric class from columns #739
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (
|
This makes all columns consistent. The GOVUK Design system documentation also suggests using this class only when comparing columns of numbers: When comparing columns of numbers, align the numbers to the right in table cells. #679
DilwoarH
force-pushed
the
679-consistent-way-of-displaying-numerical-datadates-in-table-component
branch
from
e4b53a9 to
d3a3821
Compare
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
src/middleware/dataview.middleware.js (1)
47-49: Consider adding text-align CSS class for URL columnsWhilst removing the numeric class is correct, consider whether URL columns need specific alignment for better readability. The GOVUK Design System suggests considering alignment based on content type.
if (urlRegex.test(entity[field])) { html = `<a href='${entity[field]}' target='_blank' rel='noopener noreferrer' aria-label='${entity[field]} (opens in new tab)'>${entity[field]}</a>` value = undefined + classes = 'govuk-table__cell--left' }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/middleware/dataview.middleware.js(1 hunks)src/views/components/table.html(1 hunks)test/unit/middleware/dataview.middleware.test.js(1 hunks)
🔇 Additional comments (3)
src/middleware/dataview.middleware.js (1)
42-43: LGTM: Removal of numeric class aligns with GOVUK Design System
The removal of numeric class assignment aligns with the GOVUK Design System guidance, which specifies that numeric classes should only be used when comparing columns of numbers.
test/unit/middleware/dataview.middleware.test.js (1)
95-96: LGTM: Test cases correctly validate removal of numeric classes
The test assertions have been properly updated to expect empty classes for both numeric and date fields, which aligns with the changes in the middleware and GOVUK Design System guidance.
Also applies to: 101-102
src/views/components/table.html (1)
42-42: LGTM: Simplified column definition aligns with GOVUK Design System
The removal of conditional class assignment from col elements aligns with the GOVUK Design System guidance and simplifies the template logic.
Let's verify that no other templates are expecting these classes:
✅ Verification successful
Simplified column definition is safe and maintains styling
The removal of class assignments from the col element is safe as:
- No numeric classes are used anywhere in the views
- Table styling is maintained through the proper GOV.UK Design System classes (
govuk-table,govuk-table__cell, etc.) - Custom styling is handled via the
.dl-tableclasses which don't rely on column-specific classes - Cell-level styling is still possible through the
columnData.classesparameter
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining numeric class usage in templates
# This helps ensure we've caught all instances where the class might have been expected
rg -l "govuk-table__cell--numeric" "src/views/"
Length of output: 45
Script:
#!/bin/bash
# Let's broaden our search to check for any numeric class usage across the codebase
# and also look for related table styling patterns
# Check for any numeric class patterns in the codebase
rg -i "numeric" "src/views/" -A 2
# Look for table-related class usage
rg "govuk-table" "src/views/" -A 2
# Check for any table-related styling in SCSS files
fd -e scss -x rg "table" {} -A 2
Length of output: 7460
…atadates-in-table-component
Preview link: https://submit-pr-739.herokuapp.com/organisations/local-authority:WAE/brownfield-land/data
What type of PR is this? (check all applicable)
Description
This makes all columns consistent. The GOVUK Design system documentation also suggests using this class only when comparing columns of numbers:
When comparing columns of numbers, align the numbers to the right in table cells.
#679
Related Tickets & Documents
QA Instructions, Screenshots, Recordings
Before
After
Added/updated tests?
We encourage you to keep the code coverage percentage at 80% and above.
QA sign off
[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