Skip to content
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

Html page redesign #242

Merged
merged 18 commits into from
Dec 30, 2024
Merged

Html page redesign #242

merged 18 commits into from
Dec 30, 2024

Conversation

TillK17
Copy link
Contributor

@TillK17 TillK17 commented Dec 18, 2024

Context

AI/ai-sdk-java-backlog#I128

I have redesigned the Frontend of our sample app, including endpoint descriptions (based on JavaDoc), JSON rendering (GitHub Repo of tool), toggling between full json view or only showing response message (for calls to LLM with message output), streaming functionality and minimal error handling.

Feature scope:

  • Prettify the page to look like an actual modern web page
    • Claude is your friend for this 😉
  • Add descriptions for endpoints (add links, what is OpenAI, what is orchestration service, "here we try to trigger a content filter expecting the following result")
  • Render response text content instead of plain JSON response

Definition of Done

  • Functionality scope stated & covered
  • Tests cover the scope above
  • Error handling created / updated & covered by the tests above
  • Aligned changes with the JavaScript SDK
  • Documentation updated
  • Release notes updated

@TillK17 TillK17 added the please-review Request to review a pull-request label Dec 18, 2024
@TillK17
Copy link
Contributor Author

TillK17 commented Dec 18, 2024

  • I would especially be interested in feedback regarding the explanation of the different API parts as many of you are most likely able to explain them better than me
  • as there is no final decision on our new logo yet, I've agreed with @jjtang1985 to add this later

@TillK17
Copy link
Contributor Author

TillK17 commented Dec 19, 2024

  • last commit is adapted to new backend of sample app with differentiation of JSON or content response by header field in request
  • global toggle to switch between JSON rendering or plain text output
  • not tested with new backend

a-d and others added 5 commits December 23, 2024 11:54
# Conflicts:
#	sample-code/spring-app/src/main/resources/static/index.html
# Conflicts:
#	sample-code/spring-app/src/main/resources/static/index.html
@Jonas-Isr
Copy link
Member

I merged the current state into this branch and fixed the behavior regarding the new backend. I also added minor improvements to the code.

<a class="link-offset-2-hover link-underline link-underline-opacity-0 link-underline-opacity-75-hover endpoint"
href="/deployments/by-config/67e8d039-c7f1-4179-9f8f-60d158a36b0e/createDelete"><code>/deployments/by-config/[...]/createDelete</code></a>
<div class="tooltip-content">
Create and delete a deployment with the Java specific configuration ID.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Create and delete a deployment with the Java specific configuration ID.
Create a deployment for a configuration ID and delete after.

Reasoning below.

<a class="link-offset-2-hover link-underline link-underline-opacity-0 link-underline-opacity-75-hover endpoint"
href="/deployments/by-config/67e8d039-c7f1-4179-9f8f-60d158a36b0e/stop"><code>/deployments/by-config/[...]/stop</code></a>
<div class="tooltip-content">
Stop all deployments with the Java specific configuration ID. Only RUNNING deployments can be STOPPED.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Stop all deployments with the Java specific configuration ID. Only RUNNING deployments can be STOPPED.
Stop all deployments with a specific configuration ID. Only RUNNING deployments can be STOPPED.

Configurations IDs are not tied to any language. Configurations listed are scoped under a resource group only.

Apart from that, I think, we should keep these details that are specific to our e2e test setup in aicore instance away from any user. I would remove it from all endpoint docs. Please get a second opinion about this.

newtork
newtork previously approved these changes Dec 30, 2024
Co-authored-by: Roshin Rajan Panackal <36329474+rpanackal@users.noreply.github.com>
TillK17 and others added 5 commits December 30, 2024 12:15
Co-authored-by: Roshin Rajan Panackal <36329474+rpanackal@users.noreply.github.com>
Co-authored-by: Roshin Rajan Panackal <36329474+rpanackal@users.noreply.github.com>
Co-authored-by: Roshin Rajan Panackal <36329474+rpanackal@users.noreply.github.com>
@TillK17
Copy link
Contributor Author

TillK17 commented Dec 30, 2024

  • have integrated most of @rpanackal feedback
  • first two comments that arent resolved yet are fine for me, I dont really have an opinion wether we should remove e2e test setup related details, it should just be aligned with JavaDoc of controllers
  • last open comment is resolved in 2 latest commits (as I wasn't sure about changing the Orchestration Controller JavaDoc, I did it in a seperate commit)

Copy link
Member

@rpanackal rpanackal left a 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.

@rpanackal rpanackal merged commit 8113936 into main Dec 30, 2024
5 checks passed
@rpanackal rpanackal deleted the html-page-redesign branch December 30, 2024 14:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
please-review Request to review a pull-request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants