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: Move the banner above summary, and fix the use of team hook #2859
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
✅ All tests successful. No failed tests found @@ Coverage Diff @@
## main #2859 +/- ##
=======================================
Coverage 98.48% 98.48%
=======================================
Files 878 879 +1
Lines 13027 13032 +5
Branches 3494 3474 -20
=======================================
+ Hits 12829 12834 +5
Misses 194 194
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. @@ Coverage Diff @@
## main #2859 +/- ##
=======================================
Coverage 98.48% 98.48%
=======================================
Files 878 879 +1
Lines 13027 13032 +5
Branches 3489 3492 +3
=======================================
+ Hits 12829 12834 +5
Misses 194 194
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 #2859 +/- ##
=====================================
Coverage 98.48 98.48
=====================================
Files 878 879 +1
Lines 13027 13032 +5
Branches 3489 3492 +3
=====================================
+ Hits 12829 12834 +5
Misses 194 194
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 #2859 +/- ##
=======================================
Coverage 98.48% 98.48%
=======================================
Files 878 879 +1
Lines 13027 13032 +5
Branches 3471 3497 +26
=======================================
+ Hits 12829 12834 +5
Misses 194 194
Partials 4 4
Continue to review full report in Codecov by Sentry.
|
Bundle ReportChanges will increase total bundle size by 644 bytes ⬆️
|
Bundle ReportChanges will increase total bundle size by 644 bytes ⬆️
|
✅ Deploy preview for gazebo ready!Previews expire after 1 month automatically.
|
defe05a
to
b41de14
Compare
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.
Small request, that I think should help the users out
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.
m
: I know you're moving the banner to the top, however I think it's still worth showing some form of an error message down here, instead of just an empty table, users "shouldn't" miss the banner at the top, but they might, so I think we should still leave some error message down in the table so they're at least aware of some issue occurring.
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.
Do you suggest we leave the banner there as well, or loop in Kyle for a new copy?
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.
Uhh i think new copy would probably be best imo, i would show something similar to how we currently display the other error messages ... the banner always felt out of place
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.
Or if you feel comfortable coming up with something, i think that'll be alright as well, it could be the same copy that's in the banner ... just not in the banner
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.
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.
different from other errors we display, as it's related to user onboarding (it's not even an error)
Yes, that is correct, it's not an error rather it's a moment in onboarding where the data isn't showing since it's not merged to default branch. Sounds like there are no blockers here – If there is a follow up for inconsistency, could we create an issue with the details to capture and resolve separately?
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.
Thank you @codecovdesign
@nicholas-codecov can you create an issue for the inconsistency please? Also as it is not a blocker for this PR can we move forward?
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.
Since we're introducing the inconsistency in this PR in the first place, and it's only two lines. Can we revert those two lines and bring back the assignment to the copy
var? We can then go through and update all of them together so we get to tackle the issue without introducing any inconsistency.
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.
@nicholas-codecov so delete the copy that you asked me to add?
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.
I just synced with Kyle, we think having 2 exact banners in the same page is a bit weird, so let's keep this copy for now, and i really encourage you to create the issue, i'll make sure it's tackled soon
Description
The purpose of this PR is to left the onboarding banner up the summary. The banner should be visible for the user under the coverage tab
Notable Changes
isFirstPullRequest
field within repository codecov-api#551isFirstPR
field under repo. We needed a quick way to know whether this repo has an unmerged first PR in CodecovScreenshots
closes: codecov/engineering-team#1541
Legal Boilerplate
Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. In 2022 this entity acquired Codecov and as result Sentry is going to need some rights from me in order to utilize my contributions in this PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.