Skip to content

refactor(site): refactor resource and agents #11647

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

Merged
merged 28 commits into from
Jan 19, 2024

Conversation

BrunoQuaresma
Copy link
Collaborator

Before:
Screenshot 2024-01-16 at 14 03 20

Now:
Screenshot 2024-01-16 at 14 06 22

@BrunoQuaresma BrunoQuaresma self-assigned this Jan 16, 2024
@MrPeacockNLB
Copy link
Contributor

just a quick remark. We can dynamic create coder applications. So this makes it easy for us to expose multiple microservices from one workspace. In this particular case we have a workpspace with at least 9 extra buttons across multiple lines.

Nevertheless I like the new layout

@BrunoQuaresma
Copy link
Collaborator Author

@MrPeacockNLB can you share how it looks like?

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma left a comment

Choose a reason for hiding this comment

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

Related to #11491

@BrunoQuaresma BrunoQuaresma requested review from a team, Kira-Pilot, aslilac and Parkreiner and removed request for a team January 17, 2024 16:10
const showVSCode =
(agent.display_apps.includes("vscode") ||
agent.display_apps.includes("vscode_insiders")) &&
!hideVSCodeDesktopButton;
Copy link
Member

Choose a reason for hiding this comment

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

The display code in this file is fairly complicated - there is a lot of conditional logic and you must understand the difference between the various VS Code apps and the difference between agent.apps and agent.display_apps. I think we could really benefit from some tests here, and maybe a comment or two. We have some tests for very similar logic in the AgentRowPreview component that might be worth looking at. Open to storybook tests, too...I'm just not sure how one could describe the business logic adequately through those, but interactions might help out!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is true. Thx for sharing a good test example 🙏

Copy link
Collaborator Author

@BrunoQuaresma BrunoQuaresma Jan 18, 2024

Choose a reason for hiding this comment

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

I'm not sure the best way to print the results of a "table style" test.. What do you think about this?

Screenshot 2024-01-18 at 15 21 33

@Parkreiner
Copy link
Member

Weird glitch with GitHub – all of my pending review comments got published, even though I wasn't done reviewing yet. Still planning to go through the rest of the code!

Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Still reviewing, and sorry to do something so heavy-handed, but getting rid of the mutation for the server data is really important

Copy link
Member

@Parkreiner Parkreiner left a comment

Choose a reason for hiding this comment

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

Overall, I think this all looks really good! And again, sorry for the PR rejection – I trust you, and know you wouldn't let the code go through. I just kind of panicked, especially since I already had GitHub randomly change my review comments, even just for this PR specifically

@BrunoQuaresma
Copy link
Collaborator Author

@Parkreiner @Kira-Pilot I think I solved all the comments. Let me know if there is anything open.

@BrunoQuaresma
Copy link
Collaborator Author

@Parkreiner Idk why but some of your comments were posted as "regular comments" so I was not able to reply or mark as resolved 😢

Copy link
Member

@Kira-Pilot Kira-Pilot left a comment

Choose a reason for hiding this comment

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

Thanks for working so hard on this!

@BrunoQuaresma BrunoQuaresma merged commit 1f63a11 into main Jan 19, 2024
@BrunoQuaresma BrunoQuaresma deleted the bq/imrefactor-resources-data branch January 19, 2024 12:06
@github-actions github-actions bot locked and limited conversation to collaborators Jan 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants