feat: New Setting UI. #955

Emt-lin · 2024-12-20T08:25:52Z

New Setting UI.

I tested some popular themes. It's work well in light/dark mode.

logancyang · 2024-12-28T20:43:19Z

Tailwind PR is merged! This one can rebase master now.

zeroliu · 2025-01-05T21:44:04Z

Thanks for integrating with the latest tokens! Before I dive into the implementation, I have a few pieces of feedback on the UI

Can we add a border to the slider control? It's hard to see in the following example

Can we use a neutral color for a switch when the value is false? The opaque version makes it look like it's disabled.

Can we make the tabs show text by default? The animation is nice, but it's hard to tell what each tab is about from the tab icon. There are many horizontal rooms for all the tabs to show text by default. The larger tab is also easier to use on mobile.

A list view may be more suitable for API key management. The current UI feels like people can set one API key for one provider at a time. It's also challenging to know what API keys I have set and have to pick providers one at a time to reveal the API keys. Ideally, all the providers with API keys set should show up by default.

I built an alternative version with V0. Please feel free to use it as a reference. https://v0.dev/chat/F2dDiCZXeX2?b=b_1x2gOAr4gtO

The border of this table can be removed or should use the highlight color. The gap looks off.

The grouping may benefit from some adjustments

The API key management and chat model config can live in the same tab
Configs such as "default mode", "default conversation folder name" and configs that are not AI model related can live in the "basic" tab.
Everything Embedding-related can have its tab

Can we configure temperature and context length in each AI model? How is it related to the global temperate and context length settings? This doesn't exist in the existing settings.

logancyang · 2025-01-06T01:48:35Z

@Emt-lin This is a major milestone, very nice and clean! Congrats! Once this gets released I will bump the version to v2.8 🚀 !

+1 for everything @zeroliu said. Here are some of my own initial observations:

Can we move this to the top as Plus will be our first-class citizen and we should highlight it wherever possible. Better to add a CTA button "Get Copilot Plus" that links to our landing page. Also small nit: this question mark doesn't show the tooltip on hover but only with a click.

Is "User System Prompt" missing? I don't see it on any tab.

This one should be "Chat Model[s]", and echoing @zeroliu 's comment, we don't have per-model param at the moment, so this edit button and modal shouldn't be implemented now

The provider name can get cutoff

Since we are removing a lot text explanations from the current settings (which is great), should we add a link to the doc site at the top of the settings?

logancyang · 2025-01-08T02:26:49Z

Very close to the finishing line! This is the biggest change we have before the official launch. It would be awesome if we can ship this in less than a week. Thanks for your hard work on this!! @Emt-lin

Some more comments:

Should API be its own section and not in the Chat section? Since both Chat and Embedding models require API keys. We can also add a "?" or something obvious to say "API key required for chat and QA features".

Add "default" to "chat model" and "embedding model"
If API key has its own section and we add "default" to chat and embedding model, we can remove "Chat" and "Embedding" sections and have "defaults" under "General" section.
Auto index strategy should be in the QA tab (I think we should have QA as a separate tab)
Again, we don't need these edit buttons at the moment

User System Prompt should be in Advanced settings as before

Everything below this line should be in the QA tab

Just some reorganization, overall looks great and 100x better than what I have now! 👍

src/LLMProviders/chatModelManager.ts

logancyang · 2025-01-08T02:40:22Z

src/aiParams.ts

@@ -73,6 +73,17 @@ export interface CustomModel {
  isBuiltIn?: boolean;
  enableCors?: boolean;
  core?: boolean;
+  stream?: boolean;
+  temperature?: number;
+  context?: number;


What is context? I assume you mean maxToken?

What is context? I assume you mean maxToken?

yeah, I will remove it in this PR. The next PR will implement the single model edit feature.

Great, single model parameters is a first step to the "project" feature next. Each project has its own:

Model key

Model params

User system prompt

Subset of the index with inclusions filter

logancyang · 2025-01-08T22:44:06Z

@Emt-lin The reorganization looks good! How does mobile settings work with this PR?

I'd like to merge this one asap since we are close to the official launch and we need some more user testing before it. @zeroliu let me know if you have any concerns.

logancyang · 2025-01-08T22:47:06Z

src/settings/SettingsPage.tsx

+    if (!Platform.isMobile) {
+      sections.render(<SettingsMainV2 plugin={this.plugin} />);
+      return;
+    }


Does this mean mobile is still using the old settings? Do we plan to update mobile too in the future?

Does this mean mobile is still using the old settings?

yes, maybe some componments will adopt new implementations in mobile devices.

What's the reason why mobile can't work with the new settings? It will add overhead for us to maintain two versions of settings. I would highly recommend us to keep one version if possible

@zeroliu Model list will continue to use old layout in mobile device.

I will remove the old settings.

zeroliu · 2025-01-09T05:41:09Z

src/contexts/TabContext.tsx

+  useEffect(() => {
+    // fix
+    if (!modalContainer) {
+      const modal = document.querySelector(".modal-container") as HTMLElement;
+      setModalContainer(modal);
+    }
+  }, []);


there is an eslint error here. Also what's the reason for this effect?

Portal component need container arg. eg: select、dialog、popover

Is there any reason why we need to specify the modal container as the portal container? I suggest using body directly like the current tooltip if possible. Radix uses body by default and because of obsidian special requirement, we need to attach the activeDocument body to it as containers. It will simplify the implementation a lot.

zeroliu · 2025-01-09T05:43:34Z

src/settings/SettingsPage.tsx

+    if (!Platform.isMobile) {
+      sections.render(<SettingsMainV2 plugin={this.plugin} />);
+      return;
+    }


What's the reason why mobile can't work with the new settings? It will add overhead for us to maintain two versions of settings. I would highly recommend us to keep one version if possible

src/settings/v2/components/ApiKeyDialog.tsx

src/settings/v2/components/BasicSettings.tsx

src/styles/tailwind.css

Emt-lin · 2025-01-13T07:18:12Z

@zeroliu @logancyang Embedding Model title has already modified, but I don't want to modify the implementation of the dialog. If there is a lot of feedback after launching the New setting UI, I will replace the shadcn/ui dialog with native modal .

logancyang · 2025-01-13T09:00:58Z

I think this is lgtm generally.

One potential caveat: we actually do not allow deleting core models at the moment (the code was lost from before), but that's actually a good thing because we allow deleting the "default chat model" with this new UI. Since gpt 4o is the top chat model in our builtin models, deleting a default chat model automatically sets the default to gpt 4o which is not deletable. This is an acceptable behavior now. Just wanted to point it out.

logancyang · 2025-01-13T09:02:02Z

@zeroliu let me know when you think this is ready to merge and I'll merge it.

logancyang · 2025-01-14T06:32:17Z

Merging now. Will follow up with minor updates in the future according to user feedback. Huge congrats @Emt-lin !!! 🚀🚀🚀

Emt-lin mentioned this pull request Dec 23, 2024

feat: add tailwind + shadcn/ui. #888

Merged

Emt-lin force-pushed the UI-3 branch from 167ceb4 to 9151c58 Compare January 3, 2025 03:15

Emt-lin force-pushed the UI-3 branch 2 times, most recently from d4a8623 to 0e6efaf Compare January 7, 2025 08:33

logancyang reviewed Jan 8, 2025

View reviewed changes

src/LLMProviders/chatModelManager.ts Outdated Show resolved Hide resolved

logancyang reviewed Jan 8, 2025

View reviewed changes

zeroliu reviewed Jan 9, 2025

View reviewed changes

Emt-lin force-pushed the UI-3 branch 4 times, most recently from e3409fa to ee2ea06 Compare January 10, 2025 10:50

Emt-lin added 3 commits January 13, 2025 15:10

feat: New Setting UI.

9e5ade4

feat: Optimize UX.

95bd485

feat: Continued Optimize UX.

c15a366

Emt-lin force-pushed the UI-3 branch from ee2ea06 to a82f59b Compare January 13, 2025 07:11

feat: modify Embedding Model title.

7bea14d

Emt-lin force-pushed the UI-3 branch from a82f59b to 7bea14d Compare January 13, 2025 07:24

fix: Fix password-input component.

6205c93

logancyang merged commit c3e6272 into logancyang:master Jan 14, 2025
2 checks passed

logancyang mentioned this pull request Jan 16, 2025

2.8.0 #1056

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: New Setting UI. #955

feat: New Setting UI. #955

Emt-lin commented Dec 20, 2024 •

edited

Loading

logancyang commented Dec 28, 2024

zeroliu commented Jan 5, 2025

logancyang commented Jan 6, 2025 •

edited

Loading

logancyang commented Jan 8, 2025 •

edited

Loading

logancyang Jan 8, 2025

Emt-lin Jan 8, 2025

logancyang Jan 8, 2025

logancyang commented Jan 8, 2025

logancyang Jan 8, 2025

Emt-lin Jan 9, 2025

zeroliu Jan 9, 2025

Emt-lin Jan 9, 2025

Emt-lin Jan 9, 2025

zeroliu Jan 9, 2025

Emt-lin Jan 9, 2025

zeroliu Jan 10, 2025 •

edited

Loading

zeroliu Jan 9, 2025

Emt-lin commented Jan 13, 2025

logancyang commented Jan 13, 2025

logancyang commented Jan 13, 2025

logancyang commented Jan 14, 2025

feat: New Setting UI. #955

feat: New Setting UI. #955

Conversation

Emt-lin commented Dec 20, 2024 • edited Loading

logancyang commented Dec 28, 2024

zeroliu commented Jan 5, 2025

logancyang commented Jan 6, 2025 • edited Loading

logancyang commented Jan 8, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logancyang commented Jan 8, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

zeroliu Jan 10, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emt-lin commented Jan 13, 2025

logancyang commented Jan 13, 2025

logancyang commented Jan 13, 2025

logancyang commented Jan 14, 2025

Emt-lin commented Dec 20, 2024 •

edited

Loading

logancyang commented Jan 6, 2025 •

edited

Loading

logancyang commented Jan 8, 2025 •

edited

Loading

zeroliu Jan 10, 2025 •

edited

Loading