-
Notifications
You must be signed in to change notification settings - Fork 24
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
Merged
Merged
Changes from all commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
26 changes: 26 additions & 0 deletions
26
src/pages/RepoPage/CoverageTab/FirstPullRequestBanner/FirstPullRequestBanner.spec.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
import { render, screen } from '@testing-library/react' | ||
import { MemoryRouter, Route } from 'react-router-dom' | ||
|
||
import FirstPullRequestBanner from './FirstPullRequestBanner' | ||
|
||
const wrapper: React.FC<React.PropsWithChildren> = ({ children }) => ( | ||
<MemoryRouter initialEntries={['/gh/codecov/gazebo']}> | ||
<Route path="/:provider/:owner/:repo">{children}</Route> | ||
</MemoryRouter> | ||
) | ||
|
||
describe('FirstPullRequestBanner', () => { | ||
it('renders content', () => { | ||
render(<FirstPullRequestBanner />, { wrapper }) | ||
const content = screen.getByText( | ||
'Once merged to your default branch, Codecov will show your report results on this dashboard.' | ||
) | ||
expect(content).toBeInTheDocument() | ||
}) | ||
|
||
it('renders the correct link', () => { | ||
render(<FirstPullRequestBanner />, { wrapper }) | ||
const link = screen.getByRole('link', { name: 'edit default branch' }) | ||
expect(link).toHaveAttribute('href', '/gh/codecov/gazebo/settings') | ||
}) | ||
}) |
27 changes: 27 additions & 0 deletions
27
src/pages/RepoPage/CoverageTab/FirstPullRequestBanner/FirstPullRequestBanner.tsx
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,27 @@ | ||
import A from 'ui/A' | ||
import Banner from 'ui/Banner' | ||
import BannerContent from 'ui/Banner/BannerContent' | ||
|
||
const FirstPullRequestBanner = () => { | ||
return ( | ||
<Banner> | ||
<BannerContent> | ||
<p> | ||
Once merged to your default branch, Codecov will show your report | ||
results on this dashboard.{' '} | ||
<A | ||
to={{ pageName: 'settings' }} | ||
hook={'repo-to-edit-branch'} | ||
variant="semibold" | ||
isExternal={false} | ||
data-testid="settings-page" | ||
> | ||
edit default branch | ||
</A> | ||
</p> | ||
</BannerContent> | ||
</Banner> | ||
) | ||
} | ||
|
||
export default FirstPullRequestBanner |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
export { default } from './FirstPullRequestBanner' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
went with copy with no banner (fyi banner would still show i just wanted to show you how it'll look)
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.
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