-
Notifications
You must be signed in to change notification settings - Fork 185
Change month microformat from "m" to "mo" in narrow style formatting and fix micro format tense logic #321
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
Conversation
@francinelucca 👋 This repository doesn't have Copilot instructions. With Copilot instructions, I can understand the repository better, work faster and produce higher quality PRs. I can generate a .github/copilot-instructions.md file for you automatically. Click here to open a pre-filled issue and assign it to me. I'll write the instructions, and then tag you for review. |
Co-authored-by: francinelucca <[email protected]>
Co-authored-by: francinelucca <[email protected]>
@copilot add test for a time duration that includes months and minutes, for example P1M2D3M30S |
Co-authored-by: francinelucca <[email protected]>
Added test for duration P1M2DT3M30S that includes both months and minutes. The test verifies that months display as "1mo" and minutes as "3m" in narrow style formatting, confirming the fix eliminates the ambiguity between the two units. See commit 7bf7225. |
@copilot fix test errors |
Co-authored-by: francinelucca <[email protected]>
Fixed the test errors. The issue was with the micro format tense checking logic that was causing month-level durations to fall back to minute display instead of showing the actual month duration. Updated the logic to allow month durations to pass through even with mismatched tense. All 578 tests now pass. See commit eeade77. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR resolves the ambiguity in narrow style formatting where both months and minutes were displayed as "m", making it difficult for users and assistive technologies to distinguish between the two units. The change updates months to use "mo" while minutes continue to use "m", and fixes the micro format to properly display month durations regardless of tense mismatches.
- Changed month abbreviation from "m" to "mo" in narrow style formatting to eliminate confusion with minutes
- Fixed micro format tense logic to allow month-level durations to display properly even with mismatched tense
- Updated tests and documentation to reflect the new month abbreviation
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
src/duration-format-ponyfill.ts | Added custom handling to replace "m" with "mo" for month units in narrow style |
src/relative-time-element.ts | Fixed micro format tense logic to allow month-level durations to display with mismatched tense |
test/duration-format-ponyfill.ts | Added test for complex duration showing months as "1mo" and minutes as "3m" |
test/relative-time.js | Updated test expectations for month-related cases and added new micro format test |
README.md | Updated documentation table to reflect new narrow format example |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
@@ -561,6 +561,17 @@ suite('relative-time', function () { | |||
await Promise.resolve() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot detected a code snippet with 1 occurrences. See search results for more details.
Matched Code Snippet
relative-time')
time.setAttribute('tense', 'past')
time.setAttribute('datetime', datetime)
time.setAttribute('format', 'micro')
await Promise.resolve()
assert.equal(time.shadowRoot.textContent,
Copilot uses AI. Check for mistakes.
I can't believe Copilot fixed this, amazing! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems consistent with our approach, looks good!
Closes #320, https://github.com/github/primer/issues/5575
This PR resolves the ambiguity in narrow style formatting where both months and minutes were displayed as "m", making it difficult for users and assistive technologies to distinguish between the two units.
Problem
In the narrow style formatting (
formatStyle: 'narrow'
), both months and minutes used the same abbreviation "m":This created confusion, especially for screen readers which might interpret "1m" as "1 meter" instead of the intended time unit.
Additionally, the micro format had a bug where month-level durations with mismatched tense (e.g., past dates with
tense="future"
) would incorrectly fall back to displaying "1m" (1 minute) instead of the actual month duration "1mo".Solution
Months now use "mo" while minutes continue to use "m", and the micro format properly displays month durations regardless of tense mismatches:
Changes Made
formatToParts()
to replace "m" with "mo" for month units in narrow styleImpact
Fixes #320.
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.