-
Notifications
You must be signed in to change notification settings - Fork 770
Add resource relationship to app model #5311
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
Conversation
src/Aspire.Hosting/ApplicationModel/ResourceRelationshipAnnotation.cs
Outdated
Show resolved
Hide resolved
src/Aspire.Hosting/ApplicationModel/ResourceRelationshipAnnotation.cs
Outdated
Show resolved
Hide resolved
dff7497 to
8248dae
Compare
|
From an app model perspective I am happy with where this is at except for the XML doc comments ;) |
8248dae to
82aa476
Compare
2a63a77 to
0c6bdbc
Compare
d209098 to
d62dfa5
Compare
|
I think this is ready for folks to review. @leslierichardson95 @davidfowl UI feedback is welcome on adding colors to resources page. This is the same thing the dashboard does in telemetry pages, i.e. IMO the colors back it easier to quickly identify resources, but I'm curious whether folks think it makes the resources page too busy. |
| var resource = source.Resource; | ||
| connectionName ??= resource.Name; | ||
|
|
||
| builder.WithRelationship(resource, "ConnectionString"); |
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.
WellKnownRelationshipTypes constants class?
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.
Done
|
Yeah, it doesn't look that great. However, it's the UI we're using everywell else. Buttons looking a bit ugly is why I've been switching to the action column with I'll play around with options later. I don't want to hold this PR up on a new UI design for row links. |
|
I am not a fan on the colors, I think it is too noisy indeed. |
8b78d15 to
f1c1533
Compare
|
All feedback applied. |
Personally, I'm a fan of having the colors because as you said, it helps people recognize resources quicker and it aligns with what already exists in other pages like Traces. |
leslierichardson95
left a comment
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.
Looks good to me (including the color tags)!
|
I've already removed the colors. To avoid colors and UI discussion being a blocker, this PR can be merged without them, and then we can discuss later whether they're wanted or not. Adding (or removing) them isn't difficult. |
mitchdenny
left a comment
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.
Few small things but this looks good. Either do those minor tweaks now or as a follow-up.
f1c1533 to
de4c3d5
Compare
717b833 to
73e9c3a
Compare

Description
Fixes #2595
This PR adds:
WithRelationshipextension method to app modelWithRelationshipcalls are in various methods that add or reference resources.ResourceViewModel.ReferencesandBack referencesChecklist
<remarks />and<code />elements on your triple slash comments?Microsoft Reviewers: Open in CodeFlow