Skip to content

Conversation

drewnoakes
Copy link
Member

@drewnoakes drewnoakes commented Oct 17, 2024

Description

The commas between endpoints are redundant as the underline of the anchor visually separates multiple entries. The comma visually collides with the underline, and looks a bit strange when there's only one item and the rest in overflow.

Subjective, but I think it looks a little sharper like this.

Before

image

image

After

image

image

Checklist

  • Is this feature complete?
    • Yes. Ready to ship.
    • No. Follow-up changes expected.
  • Are you including unit tests for the changes and scenario tests if relevant?
    • Yes
    • No
  • Did you add public API?
    • Yes
      • If yes, did you have an API Review for it?
        • Yes
        • No
      • Did you add <remarks /> and <code /> elements on your triple slash comments?
        • Yes
        • No
    • No
  • Does the change make any security assumptions or guarantees?
    • Yes
      • If yes, have you done a threat model and had a security review?
        • Yes
        • No
    • No
  • Does the change require an update in our Aspire docs?
    • Yes
      • Link to aspire-docs issue:
    • No
Microsoft Reviewers: Open in CodeFlow

The commas between endpoints are redundant as the underline of the anchor visually separates multiple entries. The comma visually collides with the underline, and looks a bit strange when there's only one item and the rest in overflow.

Subjective, but I think it looks a little sharper like this.
@JamesNK
Copy link
Member

JamesNK commented Oct 17, 2024

The commas between endpoints are redundant as the underline of the anchor visually separates multiple entries.

This won't be true for tcp endpoints. They're plain text.

However, maybe the space can be the delimiter. That shouldn't be a situation where an endpoint contains spaces.

@JamesNK
Copy link
Member

JamesNK commented Oct 17, 2024

@DamianEdwards @davidfowl @leslierichardson95 Opinions either way?

@drewnoakes
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes
Copy link
Member Author

This won't be true for tcp endpoints. They're plain text.

True, some URLs won't be hyperlinked or underlined, but the space should still be clear.

@DamianEdwards
Copy link
Member

Yeah I like this change

Copy link
Member

@adamint adamint left a comment

Choose a reason for hiding this comment

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

Agree that space is an acceptable delimiter here

@adamint
Copy link
Member

adamint commented Oct 17, 2024

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@davidfowl davidfowl enabled auto-merge (squash) October 17, 2024 19:06
@drewnoakes
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes
Copy link
Member Author

/azp run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes
Copy link
Member Author

@adamint the test failure here seems related to health checks. Did we commit something that breaks tests?

Assert.Null() Failure: Value of type 'Nullable<HealthStatus>' has a value
Expected: null
Actual:   Healthy

@davidfowl davidfowl merged commit 9e815f4 into main Oct 21, 2024
9 checks passed
@davidfowl davidfowl deleted the drnoakes/remove-endpoint-commas branch October 21, 2024 03:11
@github-actions github-actions bot locked and limited conversation to collaborators Nov 21, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants