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

enh: custom UI for scoped context chat #45

Merged
merged 1 commit into from
Mar 22, 2024

Conversation

kyteinsky
Copy link
Contributor

@kyteinsky kyteinsky commented Mar 4, 2024

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



image

@kyteinsky kyteinsky requested a review from julien-nc March 4, 2024 12:41
@kyteinsky kyteinsky marked this pull request as draft March 4, 2024 12:50
Copy link
Member

@jancborchardt jancborchardt left a 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? :)

@kyteinsky kyteinsky force-pushed the enh/scoped-context-chat-task-type branch 2 times, most recently from 18bb7db to 0a5aab5 Compare March 20, 2024 14:13
@julien-nc julien-nc marked this pull request as ready for review March 20, 2024 14:14
@julien-nc julien-nc force-pushed the enh/scoped-context-chat-task-type branch from 0a5aab5 to 5ecbd65 Compare March 20, 2024 14:16
@kyteinsky kyteinsky force-pushed the enh/scoped-context-chat-task-type branch from 5ecbd65 to 1f4a865 Compare March 20, 2024 14:32
Copy link
Member

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

src/components/AssistantFormInputs.vue Outdated Show resolved Hide resolved
src/components/AssistantFormInputs.vue Show resolved Hide resolved
Comment on lines 152 to 155
$inputs['scopeList'] = array_filter(
array_map(fn (array $scope) => $scope['id'] ?? null, $inputs['scopeList']),
fn ($scope) => is_string($scope),
);
Copy link
Member

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>?

Copy link
Contributor Author

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" />
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
<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.

Copy link
Contributor Author

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.

@AndyScherzinger
Copy link
Member

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

@kyteinsky kyteinsky force-pushed the enh/scoped-context-chat-task-type branch from 1f4a865 to 93b90aa Compare March 22, 2024 09:35
@kyteinsky kyteinsky force-pushed the enh/scoped-context-chat-task-type branch from 93b90aa to 2623c1e Compare March 22, 2024 09:42
@kyteinsky
Copy link
Contributor Author

@jancborchardt new screenshots with the suggested changes:

Screenshot from 2024-03-22 15-14-53
Screenshot from 2024-03-22 15-14-40

@AndyScherzinger Thanks for the heads up! I think the only svg icon we're using is folder.svg from /apps/theming/img/core/filetypes/folder.svg. This seems unaffected/the latest. Others are procured from the php side using the preview manager. So we're good.

@julien-nc julien-nc merged commit 010635f into main Mar 22, 2024
4 checks passed
@delete-merged-branch delete-merged-branch bot deleted the enh/scoped-context-chat-task-type branch March 22, 2024 11:34
@julien-nc julien-nc mentioned this pull request Mar 22, 2024
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.

4 participants