feat: add tailwind + shadcn/ui. #888

Emt-lin · 2024-12-02T11:28:13Z

Support out-of-the-box the Tailwind + shadcn/ui functionality

styles.css

src/styles/tailwind.css

logancyang · 2024-12-04T04:58:17Z

I checked out this branch and ran locally, it seems to override some obsidian native CSS vars

Emt-lin · 2024-12-04T06:26:16Z

@logancyang @zeroliu

now, style.css is already generated by tailwind cli. The original traditional css style are intailwind.css file. Perhaps We should make these traditional css style into tailwind style.

logancyang · 2024-12-04T06:43:42Z

@logancyang @zeroliu

now, style.css is already generated by tailwind cli. The original traditional css style are intailwind.css file. Perhaps We should make these traditional css style into tailwind style.

Is this going to be quite a big effort to make everything look the same/similar? I'm not familiar with how it can be converted though. @zeroliu is the expert here I'll wait for his recommendation.

Emt-lin · 2024-12-04T10:57:31Z

@logancyang @zeroliu
now, style.css is already generated by tailwind cli. The original traditional css style are intailwind.css file. Perhaps We should make these traditional css style into tailwind style.

Is this going to be quite a big effort to make everything look the same/similar? I'm not familiar with how it can be converted though. @zeroliu is the expert here I'll wait for his recommendation.

I already delete styles.css file in latest commit, it will be created by tailiwind cli When you are running npm run dev or build

src/styles/tailwind.css

package.json

logancyang · 2024-12-05T00:39:19Z

Awesome job! The UI now looks correct to me.

One concern is that the bundle size is up significantly from 5MB+ to 21MB+. Is there any way we can make it smaller?

Tailwind purging?

zeroliu

Left some more comments. Looking good overall! Looking forward to using tailwindCSS in the near future

src/styles/app.css

zeroliu · 2024-12-08T19:00:21Z

tailwind.config.js

+    preflight: false,
+  },
+  theme: {
+    extend: {


can we not extend but redefine all the tokens? Otherwise, people can still use predefined tailwind tokens without realizing that it's not connected to obsidian CSS variables. We can also turn on this eslint rule to enforce only supported classnames are used in the code base: https://github.com/francoismassart/eslint-plugin-tailwindcss/blob/master/docs/rules/no-custom-classname.md

src/components/ui/input.tsx

zeroliu · 2024-12-08T19:02:08Z

src/components/ui/input.tsx

+
+import { cn } from "@/lib/utils";
+
+const Input = React.forwardRef<HTMLInputElement, React.ComponentProps<"input">>(


not blocking this PR but maybe we can look into React 19 and use the ref props now that it's officially GA. Maintaining forwardRef for RadixUI can be pretty tedious.

logancyang · 2024-12-15T04:40:59Z

@Emt-lin Thanks for the sync today! When you get a chance, could you address zero's comments? And I think this should be ready to merge after!

zeroliu · 2024-12-21T04:51:23Z

src/constants.ts

@@ -121,7 +121,8 @@ export enum EmbeddingModelProviders {
  OPENAI = "openai",
  COHEREAI = "cohereai",
  GOOGLE = "google",
-  AZURE_OPENAI = "azure_openai",
+  // AZURE_OPENAI = "azure_openai",
+  AZURE_OPENAI = "azure openai",


does it belong to this PR?

yeah, it's a separate commit and @logancyang also learn about this.

zeroliu · 2024-12-21T04:54:52Z

src/styles/tailwind.css

+  body {
+    /* preDefine CSS variables in Obsidian.(https://docs.obsidian.md/Reference/CSS+variables/Foundations/Colors) */
+    /*Primary background*/
+    --ob-bg-primary: var(--background-primary);


why do we need to redefine obsidian CSS with --ob-bg prefix?

I think that redefine obsidian CSS will more clarify. I want to provide out-of-the-box functionality, when we add some shadcn/ui component.

Do you have any specific features in mind that would require us to break apart from the default css variables? This is an additional layer that we have to maintain. If we want to apply a new obsidian css variable that is not included in this PR, the process of adding it would require us to:

Make a copy of the css variable

Create a new tailwind token with a different semantic value

The more undocumented overhead we add, the more difficult it is for new contributors to make changes.

If you feel strongly that creating our own semantic token is necessary, and it can simplify the component library adoption, I'm supportive of that. However, we should at least consider not adding another layer of complexity and use the obsidian css variable directly.

Also worth noting that any external CSS variable that we introduce will not be compatible with the obsidian theme, which can be a dealbreaker for some users.

I am merely listing all the obsidian css variables rather than defining a new set of variables.

If we want to apply a new obsidian css variable

I have listed most of the color css variables.

The more undocumented overhead we add, the more difficult it is for new contributors to make changes.

When a new contributor joins, he/her doesn't need to do anything extra work, and he/her doesn't need to care about these.

He can use existing components or download new ones from the shadcn/ui, and then he doesn't need to do anything, the component will automatically change with the theme.

zeroliu · 2024-12-21T04:55:43Z

src/styles/tailwind.css

+  }
+}
+
+/* this layer for overwrite Obsidian default css style(file: app.css) and theme css style */


I don't think we should be touching the default component style as it will affect UI outside of obsidian-copilot

@zeroliu It doesn't affect UI outside of obsidian-copilot, It only work for copilot ui.

This will affect all matching components. I created a sample plugin and added a button with "select-trigger-reset" class. By loading this version of copilot, my settings button now looks different because of this style set in copilot plugin:

Just checked again, this is not a new problem introduced by this PR. The existing style can also affect other plugins. If I add warning-message to my plugin element and load the master version copilot, my element will also be affected. I sent a question to the obsidian discord to understand how style isolation works in obsidian. It seems very error-prone.

select-trigger-reset is my custom css classname. Others have a very low probability of using it.

This will affect all matching components.

I was aware of css conflicts when developing the UI earlier.

The reason：

zeroliu · 2024-12-21T04:56:50Z

tailwind.config.js

+import { colorOpacityPlugin } from "./src/lib/plugins/colorOpacityPlugin";
+import colors from "tailwindcss/colors";
+
+/** @type {import("tailwindcss").Config} */


we can use tailwind.config.ts to get full type support instead of this type comment

The JS type documentation also provides full type support. Therefore, both TS and JS can work.

zeroliu · 2024-12-21T04:57:53Z

tailwind.config.js

+      black: colors.black,
+      white: colors.white,


what's the use case of black, white, red, yellow? Are there existing semantic tokens that can replace them?

I think we don't need full semantic tokens support. It only base color. We don't need to fully trap ourselves in the concept of Obsidian. It's also tailwind project.

Using colors like white and black directly will likely to cause dark theme to break, as well as causing unexpected edge cases for people using non default themes. Do you have a good way to avoid these potential breakage?

I test some top theme. It's work well in light/dark mode. Dialog overlay will use the black color. I think you can pull up pr #955, you will know about something.

zeroliu · 2024-12-21T04:59:21Z

tailwind.config.js

+        3: "var(--size-4-3)",
+        4: "var(--size-4-4)",
+        5: "var(--size-4-5)",
+        5.5: "calc(var(--size-4-5) + 2px)",


When do we need this token? It doesn't feel necessary to introduce a new token that needs to calculate a new value outside of the obsidian design system?

In the button component, I need the define of 5.5, others else can be removed.

zeroliu · 2024-12-21T04:59:59Z

src/settings/model.ts

does this change belong to this PR?

Emt-lin · 2024-12-25T10:36:39Z

@logancyang @zeroliu please review this pr.

tailwind.config.js

zeroliu · 2024-12-25T19:13:04Z

tailwind.config.js

+        70: "var(--color-base-70)",
+        100: "var(--color-base-100)",
+      },
+      // red: "var(--color-red)",


can we not use the obsidian defined red and yellow color?

The defalut color offers more choices. Why we need the red color?

This is the principle of limiting the choice for developers and consistency.

Regarding limiting the choice, TailwindCSS is a generic CSS library; its goal is to allow people to build any applications. TW has to provide not only a complete set of primitive tokens and semantic tokens and let people decide what they want. Our goal is not to replicate TailwindCSS tokens but to tailor them down to what the copilot plugin may need. We don't want developers to add tokens like bg-color-red-50 without a specific reason in the plugin because #fef2f2 is not a color in either the obsidian design system or copilot design.

For consistency, this PR defines red, yellow, and black differently from the rest of the colors, making it harder to predict when applying color tokens. We would either go with Tailwind color tokens entirely or choose only obsidian colors. Staying in the middle will make decision-making harder in the future.

Unless we have a concrete use case that the obsidian color tokens cannot fit and it adds value to the copilot design, we should refrain from introducing new tokens.

Staying in the middle will make decision-making harder in the future.

agree.

But Why we need red color? Isn't it a primitive color? It's not a semantic token.

below pic -> text: yellow-700, bg: yellow-50, What should I do? eg: text-yellow-rgb/10, bg-yellow-rgb/50?

This is the callout component used in the obsidian app and site.

It uses --callout-warning which is --color-orange-rgb. The background is rgba(var(--callout-warning), 0.1)). There is no specific token for it.

My obsidian tailwind project didn't include all the component tokens from obsidian yet. You can add this token to your PR. Then you can build it as bg-callout-warning/10 and text-callout-warning. Here's the reference https://docs.obsidian.md/Reference/CSS+variables/Editor/Callout

logancyang · 2024-12-28T05:59:16Z

Just to confirm, this new setup compiles tailwind to styles.css, yes? So the release process is still to run npm run build and release main.js, styles.css and manifest.json without any additional steps, correct? @Emt-lin @zeroliu

Emt-lin · 2024-12-28T16:25:01Z

Just to confirm, this new setup compiles tailwind to styles.css, yes? So the release process is still to run npm run build and release main.js, styles.css and manifest.json without any additional steps, correct? @Emt-lin @zeroliu

yeah. cc @zeroliu

zeroliu reviewed Dec 4, 2024

View reviewed changes

styles.css Outdated Show resolved Hide resolved

zeroliu reviewed Dec 4, 2024

View reviewed changes

src/styles/tailwind.css Outdated Show resolved Hide resolved

src/styles/tailwind.css Outdated Show resolved Hide resolved

Emt-lin force-pushed the UI branch 2 times, most recently from e031a19 to a848f01 Compare December 4, 2024 10:48

logancyang reviewed Dec 5, 2024

View reviewed changes

src/styles/tailwind.css Outdated Show resolved Hide resolved

logancyang reviewed Dec 5, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

logancyang reviewed Dec 5, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

Emt-lin force-pushed the UI branch 2 times, most recently from ea9182c to 22a10f4 Compare December 5, 2024 08:51

zeroliu reviewed Dec 8, 2024

View reviewed changes

Emt-lin force-pushed the UI branch from 22a10f4 to ac0007b Compare December 20, 2024 08:20

zeroliu reviewed Dec 21, 2024

View reviewed changes

zeroliu mentioned this pull request Dec 23, 2024

Fix Radix portal #961

Merged

fix: Fix the value of EmbeddingModelProviders.AZURE_OPENAI.

7df6234

Emt-lin force-pushed the UI branch from ac0007b to b2a14b3 Compare December 25, 2024 10:35

zeroliu approved these changes Dec 25, 2024

View reviewed changes

tailwind.config.js Outdated Show resolved Hide resolved

zeroliu reviewed Dec 25, 2024

View reviewed changes

feat: add tailwind + shadcn/ui.

aa00635

Emt-lin force-pushed the UI branch from b2a14b3 to aa00635 Compare December 28, 2024 16:23

logancyang merged commit 9306f77 into logancyang:master Dec 28, 2024
2 checks passed


		import { cn } from "@/lib/utils";

		const Input = React.forwardRef<HTMLInputElement, React.ComponentProps<"input">>(

feat: add tailwind + shadcn/ui. #888

feat: add tailwind + shadcn/ui. #888

Conversation

Emt-lin commented Dec 2, 2024 • edited Loading

logancyang commented Dec 4, 2024

Emt-lin commented Dec 4, 2024

logancyang commented Dec 4, 2024

Emt-lin commented Dec 4, 2024

logancyang commented Dec 5, 2024 • edited Loading

zeroliu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logancyang commented Dec 15, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emt-lin Dec 23, 2024 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emt-lin commented Dec 25, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Emt-lin Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Emt-lin Dec 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

logancyang commented Dec 28, 2024

Emt-lin commented Dec 28, 2024

Emt-lin commented Dec 2, 2024 •

edited

Loading

logancyang commented Dec 5, 2024 •

edited

Loading

Emt-lin Dec 23, 2024 •

edited

Loading

Emt-lin Dec 26, 2024 •

edited

Loading

Emt-lin Dec 26, 2024 •

edited

Loading