-
Notifications
You must be signed in to change notification settings - Fork 26
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
DEV: converts llm admin page to use form kit #1099
base: main
Are you sure you want to change the base?
Conversation
This also converts the quota editor, and the quota modal. The most debatable change is that I moved tooltips into descriptions, we can change this in the future if that's a significant issue.
async testConfig() { | ||
return await ajax(`/admin/plugins/discourse-ai/ai-llms/test.json`, { | ||
data: { ai_llm: this.createProperties() }, | ||
async testConfig(llmConfig) { |
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.
I allowed to pass an arbitrary llmConfig object to test
@@ -1,178 +0,0 @@ | |||
import Component from "@glimmer/component"; |
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.
I just moved all of this in the form, I could split it in multiple components, but there was not much value to it. It's implemented using the collection from form kit: https://meta.discourse.org/t/discourse-toolkit-to-render-forms/326439?silent=true
@@ -4,38 +4,15 @@ import BackButton from "discourse/components/back-button"; | |||
import AiLlmEditorForm from "./ai-llm-editor-form"; | |||
|
|||
export default class AiLlmEditor extends Component { | |||
constructor() { |
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.
moved all of this in the form as it's technically the form data
@@ -110,8 +77,8 @@ | |||
grid-template-columns: repeat(auto-fill, minmax(16em, 1fr)); | |||
gap: 1em 2em; | |||
margin-top: 1em; | |||
border-top: 3px solid var(--primary-low); // matches tbody border |
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.
I had a super weird behavior in my editor with this comment not sure why, removed it for now
@@ -73,6 +71,49 @@ | |||
expect(llm.user_id).not_to be_nil | |||
end | |||
|
|||
context "with quotas" do |
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.
added simple tests for quotas
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.
LGTM! One small suggestion
openAddQuotaModal() { | ||
this.modalIsVisible = true; |
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.
Is there a particular reason why this is being switched to using the modal
service instead of the declarative technique in the template? I believe declarative technique is preferred when possible (ref).
Can we mut
a tracked property for addItemToCollection
that gets passed as an arg
to the modal
?
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.
why would it be preferred here? It's a lot more code to be declarative here, you even have to use your own tracked property.
assets/javascripts/discourse/components/modal/ai-llm-quota-modal.gjs
Outdated
Show resolved
Hide resolved
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.
Amazing 👏
…al.gjs Co-authored-by: Martin Brennan <[email protected]>
This also converts the quota editor, and the quota modal.
The most debatable change is that I moved tooltips into descriptions, we can change this in the future if that's a significant issue.