-
Notifications
You must be signed in to change notification settings - Fork 44
fix: elision of strings in table cells #2288
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
base: main
Are you sure you want to change the base?
Conversation
codap-v3
|
||||||||||||||||||||||||||||
| Project |
codap-v3
|
| Branch Review |
CODAP-1072-case-table-elision
|
| Run status |
|
| Run duration | 08m 04s |
| Commit |
|
| Committer | Kirk Swenson |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
|
0
|
|
|
1
|
|
|
48
|
|
|
0
|
|
|
301
|
| View all changes introduced in this branch ↗︎ | |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2288 +/- ##
==========================================
+ Coverage 86.53% 86.55% +0.02%
==========================================
Files 723 723
Lines 38711 38719 +8
Branches 9194 9584 +390
==========================================
+ Hits 33499 33514 +15
+ Misses 5203 5196 -7
Partials 9 9
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 encountered an error and was unable to review this pull request. You can try again by re-requesting a review.
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
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| export const kDefaultRowHeight = 18 | ||
| // used for row resizing | ||
| export const kRowHeightPadding = 4 | ||
| export const kSnapToLineHeight = 14 |
Copilot
AI
Jan 14, 2026
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.
The value of kDefaultRowHeight has changed from 18 to 18 (14 + 4). While mathematically equivalent, this change makes the constant dependent on other constants. Consider adding a comment explaining why kDefaultRowHeight equals kSnapToLineHeight + kRowHeightPadding to clarify the relationship between these values.
| export const kSnapToLineHeight = 14 | |
| export const kSnapToLineHeight = 14 | |
| // Default row height is one snap line plus vertical padding so changes to either | |
| // kSnapToLineHeight or kRowHeightPadding intentionally affect the default height. |
| })) | ||
|
|
||
| jest.mock("../../utilities/date-iso-utils", () => ({ | ||
| isStdISODateString: jest.fn((str: string) => /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z$/.test(str)) |
Copilot
AI
Jan 14, 2026
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.
The regex pattern is missing an escape for the literal dot before milliseconds. The pattern /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z$/ will match any character where the dot is. It should be /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/ to match the literal dot in ISO date strings.
| isStdISODateString: jest.fn((str: string) => /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}.\d{3}Z$/.test(str)) | |
| isStdISODateString: jest.fn((str: string) => /^\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}\.\d{3}Z$/.test(str)) |
[CODAP-1072] Case table should elide (with ...) text that doesn't fit
Also added a Copilot-written test suite for the
renderAttributeValue()function.