-
Notifications
You must be signed in to change notification settings - Fork 17
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
fix: Scroll to line padding #2814
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #2814 +/- ##
=======================================
Coverage 98.46% 98.47%
=======================================
Files 871 871
Lines 12676 12686 +10
Branches 3332 3388 +56
=======================================
+ Hits 12482 12492 +10
Misses 190 190
Partials 4 4
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found Additional details and impacted files@@ Coverage Diff @@
## main #2814 +/- ##
=======================================
Coverage 98.46% 98.47%
=======================================
Files 871 871
Lines 12676 12686 +10
Branches 3395 3388 -7
=======================================
+ Hits 12482 12492 +10
Misses 190 190
Partials 4 4
Continue to review full report in Codecov by Sentry.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found Additional details and impacted files@@ Coverage Diff @@
## main #2814 +/- ##
=====================================
Coverage 98.47 98.47
=====================================
Files 871 871
Lines 12676 12686 +10
Branches 3332 3388 +56
=====================================
+ Hits 12482 12492 +10
Misses 190 190
Partials 4 4
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 777 bytes ⬆️
|
Bundle ReportChanges will increase total bundle size by 777 bytes ⬆️
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
@@ -88,12 +92,21 @@ export const CODE_RENDERER_INFO = { | |||
} as const | |||
|
|||
// Enum from https://github.com/codecov/shared/blob/master/shared/utils/merge.py#L275-L279 |
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.
nit: think this comment is out of date if we can update
type LineContent = { types: Array<string>; content: string } | ||
|
||
export interface DiffLineProps { | ||
lineContent?: Array<LineContent> |
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.
curious on this changing from a required prop when we were using PropTypes to optional
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.
Typo
function DiffLine({ | ||
type LineContent = { types: Array<string>; content: string } | ||
|
||
export interface DiffLineProps { |
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.
nit: thoughts on organizing props in required -> optional order?
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.
Updating to be alphabetical, follows the same pattern as the auto-complete
headCoverage={coverage[Math.floor(Math.random() * 3) % 3]} | ||
baseCoverage={coverage[Math.floor(Math.random() * 3) % 3]} | ||
headCoverage={ | ||
coverage[Math.floor(Math.random() * 3) % 3] ?? null |
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.
won't coverage always be a truthy value here?
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.
No, it can be undefined
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.
Nothing blocking, just a couple comments
Description
This PR cleans up some of the instances of
useScrollToLine
to fix the padding issues with headers, when both BA and Coverage are enabled, and to use a shared const throughout the app so we only have to adjust the values in one place.Notable Changes
DiffLine
to TS and to acceptstickyPadding
propstickyPadding
to use const obj