Skip to content

Conversation

@ElderMatt
Copy link
Contributor

@ElderMatt ElderMatt commented Mar 12, 2025

Also see the other PRs:

Considerations

  • I have tested the changes in both light and dark mode.
  • I have considered the need for new unit tests.
  • I have tested the changes on a cluster.
  • I have included relevant documentation updates.
  • I have an approved Figma design or have reflected my changes in Figma
  • I have verified that the UI/UX is consistent in major browsers (e.g., Chrome, Firefox, Safari, Edge).
  • I have tested the changes for responsiveness in different screen resolutions.
  • I have tested expected error states and verified that the user is presented with informative error messages.
  • I have tested the feature with unusual or extreme inputs (e.g., very long strings, empty states, clicking a button multiple times quickly).

@dennisvankekem dennisvankekem self-assigned this Mar 12, 2025
Copy link
Collaborator

@dennisvankekem dennisvankekem left a comment

Choose a reason for hiding this comment

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

first pass, UI test will follow later

@dennisvankekem
Copy link
Collaborator

image

I'm missing the title and description here (domain aliases CNAME)

@dennisvankekem
Copy link
Collaborator

image

traffic management can go over 100%

@dennisvankekem
Copy link
Collaborator

image

I'm missing the service url summary next to the submit button

@dennisvankekem
Copy link
Collaborator

image

creating a service without filling in namespace returns an error instead of a form valiation error in the frontend

Copy link
Collaborator

@dennisvankekem dennisvankekem left a comment

Choose a reason for hiding this comment

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

first ui pass, please see comments

@ElderMatt
Copy link
Contributor Author

ElderMatt commented Apr 16, 2025

traffic management still has the 50/50 default bug

  • Fixed

Copy link
Collaborator

@ferruhcihan ferruhcihan left a comment

Choose a reason for hiding this comment

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

On Thursday, we worked together with @ElderMatt and @dennisvankekem and tested it, but we weren’t able to expose the services consistently.
Aside from that, the code looks good. The new components are functioning as expected, so it’s LGTM from my side.

Copy link
Collaborator

@dennisvankekem dennisvankekem left a comment

Choose a reason for hiding this comment

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

well done, this was a hard one

@ElderMatt ElderMatt enabled auto-merge (squash) April 23, 2025 13:45
@ElderMatt ElderMatt removed the request for review from merll April 23, 2025 13:45
@ElderMatt ElderMatt disabled auto-merge April 23, 2025 13:46
@ElderMatt ElderMatt merged commit ccbc6df into main Apr 23, 2025
2 checks passed
@ElderMatt ElderMatt deleted the APL-537 branch April 23, 2025 13:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants