Skip to content

Remove commas between endpoints in grid#6359

Merged
davidfowl merged 2 commits into
mainfrom
drnoakes/remove-endpoint-commas
Oct 21, 2024
Merged

Remove commas between endpoints in grid#6359
davidfowl merged 2 commits into
mainfrom
drnoakes/remove-endpoint-commas

Conversation

@drewnoakes

@drewnoakes drewnoakes commented Oct 17, 2024 •
edited by dotnet-policy-service Bot
Loading

Copy link
Copy Markdown
Member

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

@drewnoakes
Remove commas between endpoints in grid
d4c586b 
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.
@drewnoakes
drewnoakes requested review from JamesNK and adamint October 17, 2024 12:48
@JamesNK

JamesNK commented Oct 17, 2024

Copy link
Copy Markdown
Member

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

JamesNK commented Oct 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

@DamianEdwards @davidfowl @leslierichardson95 Opinions either way?

@drewnoakes

drewnoakes commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

azure-pipelines Bot commented Oct 17, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes

drewnoakes commented Oct 17, 2024

Copy link
Copy Markdown
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

DamianEdwards commented Oct 17, 2024

Copy link
Copy Markdown
Member

Yeah I like this change

adamint
adamint approved these changes Oct 17, 2024
View reviewed changes

@adamint adamint left a comment

Copy link
Copy Markdown
Member

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

adamint commented Oct 17, 2024

Copy link
Copy Markdown
Member

/azp run

@azure-pipelines

azure-pipelines Bot commented Oct 17, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

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

drewnoakes commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

azure-pipelines Bot commented Oct 17, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes

drewnoakes commented Oct 18, 2024

Copy link
Copy Markdown
Member Author

/azp run

@azure-pipelines

azure-pipelines Bot commented Oct 18, 2024

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@drewnoakes

drewnoakes commented Oct 18, 2024

Copy link
Copy Markdown
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
@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.

Reviewers

@adamint adamint adamint approved these changes
@JamesNK JamesNK Awaiting requested review from JamesNK JamesNK is a code owner

Assignees

No one assigned

Labels

area-dashboard

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@drewnoakes @JamesNK @DamianEdwards @adamint @davidfowl