Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
BrunoQuaresma merged 28 commits intomainfrombq/imrefactor-resources-data
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

matifali reacted with heart emoji
@BrunoQuaresmaBrunoQuaresma self-assigned thisJan 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
CollaboratorAuthor

@MrPeacockNLB can you share how it looks like?

Copy link
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresma 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

@BrunoQuaresmaBrunoQuaresma requested review froma team,Kira-Pilot,aslilac andParkreiner and removed request fora teamJanuary 17, 2024 16:10
constshowVSCode=
(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 betweenagent.apps andagent.display_apps. I think we could really benefit from some tests here, and maybe a comment or two. We have sometests for very similar logic in theAgentRowPreview 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!

BrunoQuaresma reacted with thumbs up emoji
Copy link
CollaboratorAuthor

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
CollaboratorAuthor

@BrunoQuaresmaBrunoQuaresmaJan 18, 2024•
edited
Loading

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

Kira-Pilot reacted with thumbs up emojiKira-Pilot reacted with hooray emoji
@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!

Parkreiner
Parkreiner previously requested changesJan 18, 2024
Copy link
Member

@ParkreinerParkreiner left a comment•
edited
Loading

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

@ParkreinerParkreiner 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
CollaboratorAuthor

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

@BrunoQuaresma
Copy link
CollaboratorAuthor

@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-PilotKira-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!

@BrunoQuaresmaBrunoQuaresma merged commit1f63a11 intomainJan 19, 2024
@BrunoQuaresmaBrunoQuaresma deleted the bq/imrefactor-resources-data branchJanuary 19, 2024 12:06
@github-actionsgithub-actionsbot locked and limited conversation to collaboratorsJan 19, 2024
Sign up for freeto subscribe to this conversation on GitHub. Already have an account?Sign in.
Reviewers

@Kira-PilotKira-PilotKira-Pilot approved these changes

@aslilacaslilacAwaiting requested review from aslilacaslilac is a code owner

@ParkreinerParkreinerAwaiting requested review from ParkreinerParkreiner is a code owner

Assignees

@BrunoQuaresmaBrunoQuaresma

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

4 participants
@BrunoQuaresma@MrPeacockNLB@Parkreiner@Kira-Pilot
[8]ページ先頭
©2009-2025 Movatter.jp