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

copilot: Introduced support for organizations and team metrics visualization #1261

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

esw-afabiano
Copy link
Contributor

@esw-afabiano esw-afabiano commented Sep 19, 2024

Hey, I just made a Pull Request!

This update introduces support for both organizations and team metrics visualization in the GitHub Copilot plugin. With these new features, users can now select specific teams within their organization and compare their metrics with overall data, providing more insightful analysis and management capabilities.

Additionally 1: the images included in this update, which showcase various visual aspects of the plugin, were generated using the GPT AI model. To the best of our knowledge, these images are free to use, as they were created by the AI without any existing copyright restrictions.

Additionally 2: this PR also fixes the styles within the GitHub Copilot plugin to ensure that they work seamlessly with the Backstage theme system. The components should now behave consistently, adapting to the current theme (light or dark) without issues, aligning with the other components in Backstage.

✔️ Checklist

  • A changeset describing the change and affected packages. (more info)
  • Added or updated documentation
  • Tests for new functionality and regression tests for bug fixes
  • Screenshots attached (for UI changes)
  • All your commits have a Signed-off-by line in the message. (more info)

@backstage-goalie
Copy link
Contributor

backstage-goalie bot commented Sep 19, 2024

Changed Packages

Package Name Package Path Changeset Bump Current Version
app workspaces/copilot/packages/app none v0.0.6
@backstage-community/plugin-copilot-backend workspaces/copilot/plugins/copilot-backend major v0.1.4
@backstage-community/plugin-copilot-common workspaces/copilot/plugins/copilot-common major v0.2.2
@backstage-community/plugin-copilot workspaces/copilot/plugins/copilot major v0.2.3

@esw-afabiano esw-afabiano changed the title add support for orgs and team view copilot: Introduced support for organizations and team metrics visualization Sep 19, 2024
@backstage-goalie
Copy link
Contributor

Thanks for the contribution!
All commits need to be DCO signed before they are reviewed. Please refer to the the DCO section in CONTRIBUTING.md or the DCO status for more info.

Copy link

@ScottGuymer ScottGuymer left a comment

Choose a reason for hiding this comment

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

Some initial thoughts from trying this plugin out in our instance.

The styling looks good, however, it needs to respect the global theme provider so that it better fits in with the styling of the instance.

workspaces/copilot/packages/app/src/App.tsx Outdated Show resolved Hide resolved
import { styled } from '@mui/material/styles';
import { Metric } from '@backstage-community/plugin-copilot-common';

const CardBox = styled(Box)(() => ({

Choose a reason for hiding this comment

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

There seems to be something about this way of styling components that seems to "leak" out from this plugin and affect unrelated components in the rest of the instance that it is installed into.

Im not an expert in this so I can't say what it is. @awanlin can you shed some light on this?

@ScottGuymer
Copy link

Im not sure how or why but this plugin manages to nuke my homepage when i install it

image

@awanlin
Copy link
Contributor

awanlin commented Sep 22, 2024

Hi @esw-afabiano, would it be possible to break this up? Ideally one PR focused on the initial feature and then another for the theme and style changes? There also seems to be GitHub integration/token changes. These large PRs are hard to review and lead to what you are experiencing where you are having to do follow up work based on Adopter feedback.

Also, can you post a screenshot of the images in use? Are they really needed? They don't really fit into the look and feel of Backstage and images are not really used like this anywhere. I realize I'm being the "no fun police" but a lot of Orgs are going to be frustrated by this as they have specific brand standards they follow. Maybe this is opt in?

@esw-afabiano
Copy link
Contributor Author

Hi @esw-afabiano, would it be possible to break this up? Ideally one PR focused on the initial feature and then another for the theme and style changes? There also seems to be GitHub integration/token changes. These large PRs are hard to review and lead to what you are experiencing where you are having to do follow up work based on Adopter feedback.

Also, can you post a screenshot of the images in use? Are they really needed? They don't really fit into the look and feel of Backstage and images are not really used like this anywhere. I realize I'm being the "no fun police" but a lot of Orgs are going to be frustrated by this as they have specific brand standards they follow. Maybe this is opt in?

I can certainly break this up, but I'll need to wait for this PR to be merged first. There have been significant changes to the project, and the style updates are based on the new layout. If you’d like me to separate them, I can do that!

Regarding the image, I felt that there wasn't much creativity in designing that screen, so I decided to include an image. I've also added a GIF that shows its usage: demo.gif.

Let me know if you need any adjustments!

@nickboldt
Copy link
Collaborator

Needs rebase and maybe to split into smaller PRs?

@esw-afabiano
Copy link
Contributor Author

@awanlin, regarding the issue raised about the image, does it make sense as it is now, or do we need to change it?

As for splitting this into two PRs, one for the feature and another for the theme fix, as I mentioned earlier, it can be done. However, the structure has been significantly altered. This also ties into the point raised by @nickboldt about breaking it into smaller PRs. The complicating factor is that one part depends on the other, and splitting it into smaller PRs would likely break the application. We already have a functional version of this, so I'm not sure if it's the right approach for this PR. I'm willing to do whatever is necessary, but I also want to avoid creating additional problems and rework.

Looking forward to the next steps and seeing this version used by the community!

@esw-afabiano esw-afabiano force-pushed the af/new-features-copilot branch 2 times, most recently from 8df0d1c to 4ea6947 Compare October 16, 2024 20:14
@esw-afabiano
Copy link
Contributor Author

@vinzscam, @awanlin Do you have any idea what's going on here? Locally, the command yarn install --immutable worked normally. Here, it seems to be throwing an error without detail.

@vinzscam
Copy link
Member

@vinzscam, @awanlin Do you have any idea what's going on here? Locally, the command yarn install --immutable worked normally. Here, it seems to be throwing an error without detail.

I think if you rollback the changes made to the yarn.lock at the root of the project it should fix the issue!

@esw-afabiano
Copy link
Contributor Author

@vinzscam, @awanlin Do you have any idea what's going on here? Locally, the command yarn install --immutable worked normally. Here, it seems to be throwing an error without detail.

I think if you rollback the changes made to the yarn.lock at the root of the project it should fix the issue!

did not work!

@vinzscam
Copy link
Member

vinzscam commented Oct 21, 2024

@vinzscam, @awanlin Do you have any idea what's going on here? Locally, the command yarn install --immutable worked normally. Here, it seems to be throwing an error without detail.

I think if you rollback the changes made to the yarn.lock at the root of the project it should fix the issue!

did not work!

I still see some changes to the yarn.lock at the root of the community-plugins project (not under copilot's workspace)

Signed-off-by: Alisson Fabiano <[email protected]>
Signed-off-by: Alisson Fabiano <[email protected]>
Signed-off-by: Alisson Fabiano <[email protected]>
Signed-off-by: Alisson Fabiano <[email protected]>
@esw-afabiano
Copy link
Contributor Author

@vinzscam Well.. the resolution was to have to uninstall my yarn package and reinstall using npm in version 1.22.19, and then it was able to get the yarn version defined in the project. It seems to be 100% now

@esw-afabiano
Copy link
Contributor Author

Hey @vinzscam, any chance to merge this?

Signed-off-by: Alisson Fabiano <[email protected]>
@yoramshai
Copy link

It would be great to have it released 🙏🏻

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.

6 participants