Skip to content
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

feat: Overhaul popup menu and provider management #103

Merged
merged 4 commits into from
Dec 16, 2024

Conversation

spalmurray-codecov
Copy link
Collaborator

@spalmurray-codecov spalmurray-codecov commented Dec 12, 2024

This PR is primarily meant to fix a bug where github enterprise URLs would not actually work due to 'github' being the hardcoded provider on API requests. While I was in here though I took it as a chance to rewrite the popup menu to address some other issues.

Goals for the new popup:

  1. Better support API tokens for cloud codecov
  2. Improve error messaging
  3. More clearly communicate optional settings
  4. Remove unnecessary dependencies

I think this PR was successful in achieving each of these goals! Have approved and iterated on design with input from Adalene.

Am currently testing everything actually works on a real Github enterprise instance and will not merge before confirming.

This is what the final result looks like:

Recording 2024-12-12 at 16 46 40

Here's what it currently looks like for comparison:

Screenshot 2024-12-12 at 16 47 36

@spalmurray-codecov spalmurray-codecov marked this pull request as ready for review December 12, 2024 21:49
Copy link

@nicholas-codecov nicholas-codecov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved, pointed out a couple of console.log's that may have gotten left behind tho not sure if you want them in or not

src/popup/main.tsx Outdated Show resolved Hide resolved
src/popup/main.tsx Outdated Show resolved Hide resolved
src/popup/main.tsx Outdated Show resolved Hide resolved
Copy link
Contributor

@ajay-sentry ajay-sentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rubber stamping ✅

@spalmurray-codecov spalmurray-codecov merged commit 0912732 into main Dec 16, 2024
1 check passed
@spalmurray-codecov spalmurray-codecov deleted the spalmurray/service-and-settings-overhaul branch December 16, 2024 18:36
@spalmurray-codecov
Copy link
Collaborator Author

Was not able to test this on GHE because our instance is years out of date and updating is non-trivial. We've decided to merge as-is as this won't break anything that currently works. Will need to follow up on the GHE story should we hear from our customers that it's not working.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants