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

Tailwind config support #356

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

naman1608
Copy link
Contributor

A try at #226. Tried for a few images, the code given is using the config like in the docs https://tailwindcss.com/docs/installation/play-cdn and the code does use the custom styles, but the prompt can be improved I think.

Copy link
Owner

@abi abi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left a few comments.

I'm also unable to see it working in action. When I tried it, I got the same visual results as without Tailwind config except that the code now include the Tailwind config. Can you share the test data you tried? I have a extensive list of images I use for testing, but they are not paired with a tailwind config file.

@@ -49,7 +67,7 @@ function SettingsDialog({ settings, setSettings }: Props) {
<div className="flex items-center space-x-2">
<Label htmlFor="image-generation">
<div>DALL-E Placeholder Image Generation</div>
<div className="font-light mt-2 text-xs">
<div className="mt-2 text-xs font-light">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these linter changes? What are you using?

And please revert it as part of this PR. We can make style changes but it shouldn't be mixed into a PR that adds functionality.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Needs to be obvious whether Tailwind config is enabled, and need to provide a way to remove the previously added Tailwind config or toggle it off.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are these linter changes? What are you using?

And please revert it as part of this PR. We can make style changes but it shouldn't be mixed into a PR that adds functionality.

I was using prettier and eslint, but even after disabling all extensions I'm not able to fix this. As soon as I revert to the old order it changes again.

@@ -218,6 +236,27 @@ function SettingsDialog({ settings, setSettings }: Props) {
</AccordionContent>
</AccordionItem>
</Accordion>

<Accordion type="single" collapsible className="w-full">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good for a first version. But from a UX perspective, I think this needs to be moved here at some point. I'll come up with a design for it.

Screenshot 2024-06-10 at 1 10 07 PM

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have attempted shifting it here if this is better

@@ -613,7 +613,7 @@ function App() {
{(appState === AppState.CODING || appState === AppState.CODE_READY) && (
<div className="ml-4">
<Tabs defaultValue="desktop">
<div className="flex justify-between mr-8 mb-4">
<div className="flex justify-between mb-4 mr-8">
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please revert these changes so it's easier to review the PR.

backend/prompts/__init__.py Outdated Show resolved Hide resolved
backend/prompts/__init__.py Outdated Show resolved Hide resolved
@@ -15,17 +15,49 @@
Generate code for a SVG that looks exactly like this.
"""

TAILWIND_USER_PROMPT = """
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to modify the system prompt when using Tailwind config I think. The goal with using Tailwind config is to have the generated code use the theme but the system prompt explicitly instructs the AI to replicate exactly the screenshot.

@abi
Copy link
Owner

abi commented Jun 10, 2024

@naman1608 also if you're up for it, let's do a call.

@naman1608
Copy link
Contributor Author

Left a few comments.

I'm also unable to see it working in action. When I tried it, I got the same visual results as without Tailwind config except that the code now include the Tailwind config. Can you share the test data you tried? I have a extensive list of images I use for testing, but they are not paired with a tailwind config file.

I tried 2-3 screenshots of a tailwind website I had made for another project, won't be able to share that sorry for that. I'll make a list of images paired with tailwind config for testing.

@naman1608
Copy link
Contributor Author

@naman1608 also if you're up for it, let's do a call.

That would be great!

Comment on lines +20 to +24
<script>
tailwind.config = {
// Add the configuration here
}
</script>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it have presets support?

tailwind.config = {
  presets: [require('xxx')]
}

If not, any future plan for this?

@naman1608
Copy link
Contributor Author

Screenshot 2024-06-12 at 1 39 13 PM Screenshot 2024-06-12 at 11 20 27 AM Screenshot 2024-06-12 at 11 59 36 AM Screenshot 2024-06-12 at 12 40 39 PM I tried making some sample pages paired with tailwind config, it is able to pick the colors and fonts and use those custom classes. Any other thing we should check on?

@abi
Copy link
Owner

abi commented Jul 6, 2024

@naman1608 Sorry for missing this. If you're interested, I'm still up for chatting, can you email me at the email in my Github profile and we'll set up a time.

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.

3 participants