-
Notifications
You must be signed in to change notification settings - Fork 6
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
enh: custom UI for scoped context chat #45
Conversation
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.
Scoped Context Chat and Context Chat don't need to be separate entries, they could be the same?
And Context Chat could just have a button on the right of the heading+subline saying e.g. "Pick folder or storage" which then opens an action menu.
@nimishavijay for more specific proposals with mockup, if you have firther ideas? :)
18bb7db
to
0a5aab5
Compare
0a5aab5
to
5ecbd65
Compare
5ecbd65
to
1f4a865
Compare
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, a few questions.
lib/Service/AssistantService.php
Outdated
$inputs['scopeList'] = array_filter( | ||
array_map(fn (array $scope) => $scope['id'] ?? null, $inputs['scopeList']), | ||
fn ($scope) => is_string($scope), | ||
); |
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.
Will the scope list be loaded correctly when displaying an existing task?
Wouldn't it be simpler to keep the structure of $inputs['scopeList']
intact so it is straightforward to load it in <ContextChatInputForm>
?
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.
hmm, it did work in the Assistant modal but noticed just now that it does not when pulled from the result notification. Will de-couple the UI and actual data shortly.
<NcCheckboxRadioSwitch :checked.sync="sccEnabled"> | ||
{{ t('assistant', 'Reduce context') }} | ||
</NcCheckboxRadioSwitch> | ||
<ContextChatInputForm v-if="sccEnabled" v-model="sccData" /> |
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.
<ContextChatInputForm v-if="sccEnabled" v-model="sccData" /> | |
<ContextChatInputForm v-if="sccEnabled" :scc-data.sync="sccData" /> |
But maybe you had good reasons to use v-model.
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.
not really, would switch to this for consistency. Just discovered that they do the same thing.
Just as a side note, can't tell where mimetype icons are fetched from, we are currently in the process of updating the icons throughout the system(s) server and clients, see nextcloud/server#44340 targeting the v29.0.0 release - fyi @kyteinsky @julien-nc |
1f4a865
to
93b90aa
Compare
Signed-off-by: Anupam Kumar <[email protected]>
93b90aa
to
2623c1e
Compare
@jancborchardt new screenshots with the suggested changes: @AndyScherzinger Thanks for the heads up! I think the only svg icon we're using is folder.svg from |
Design feedback required on the selector UI (Select File/Folder-Select Providers). It looks a bit too big imo. The same selector is present in speech-to-text UI so that would also need to be changed to maintain consistency. Something less obstructing/with less occupancy on the dialog is what I imagine would be a good fit.
@nimishavijay @jancborchardt