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

wip: Add variant capability #812

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mpourismaiel
Copy link
Member

What this PR does / why we need it:

Special notes for your reviewer:

Release note:


@mpourismaiel mpourismaiel requested a review from stp-ip July 25, 2020 15:56
@mpourismaiel mpourismaiel self-assigned this Jul 25, 2020
@stp-ip
Copy link
Member

stp-ip commented Jul 30, 2020

Interesting solution to trim the prefix. That way we can migrate to bootstrap/ without impact or at least none that I can think of. Kinda like that solution, but this needs a comment or at the very least a commit message explanation on why this was done.

@@ -0,0 +1,6 @@
module.exports = {
plugins: [
require('tailwindcss'),
Copy link
Member

Choose a reason for hiding this comment

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

So tailwind needs postcss to function?
Is this a full requisite or mainly for changes?
I assume this will lead to errors on the user side, if they don't have postcss-cli installed again.
So hugo (non extended) should work, hugo extended without postcss might fail and hugo extended with the right packages should be fine. Any thoughts?

@@ -0,0 +1,36 @@
const colors = {
Copy link
Member

Choose a reason for hiding this comment

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

So tailwind.config.js would be in addition to style changes within config.toml? That sounds a bit more advanced. Can we grab the colors from config.toml instead?
Any other way to reduce the need to have a tailwind.config.js?

{{- $sassTemplate := resources.Get "styles/index.scss" -}}
{{- $options := (dict "targetPath" "style.css" "outputStyle" "compressed" "enableSourceMap" true "includePaths" (slice "node_modules" "styles")) -}}
{{- $styleTemplated := $sassTemplate | resources.ExecuteAsTemplate "main.scss" . -}}
{{- $style := $styleTemplated | resources.ToCSS $options | resources.PostCSS | resources.Minify | resources.Fingerprint | resources.PostProcess }}
Copy link
Member

Choose a reason for hiding this comment

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

Note: PostCSS (needs to integrate the changes from #816, when merged)

{{- $sassTemplate := resources.Get "styles/tailwind/index.scss" -}}
{{- $options := (dict "targetPath" "tailwind.css" "outputStyle" "compressed" "enableSourceMap" true "includePaths" (slice "node_modules" "styles")) -}}
{{- $styleTemplated := $sassTemplate | resources.ExecuteAsTemplate "main.scss" . -}}
{{- $style := $styleTemplated | resources.ToCSS $options | resources.PostCSS | resources.PostProcess }}
Copy link
Member

Choose a reason for hiding this comment

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

Note: PostCSS (needs to integrate the changes from #816, when merged)

@stp-ip
Copy link
Member

stp-ip commented Aug 5, 2020

Also not sure, why the modal close "x" is being shown in firefox bottom right corner on the tailwind dev page:
Screenshot_2020-08-05 Tailwind page

Also fascinating that it does look similar to bootstrap :)

@mpourismaiel
Copy link
Member Author

This was just to try and implement a simpler fragment using the same design but using Tailwind. I have to figure out a way to simplify the "install" process. If the user doesn't have to do anything besides loading the fragment and configuring it, I'm way happier. If it's possible, move the tailwind config and try and figure out a way to handle postcss in another way. But still this PR is to show how a fragment using tailwind will be developed and used.

Theme of the fragments need to be different IMO. Or we can implement the same designs and allowing the user to choose between them, kind of a basis for custom fragments, but I don't like that idea. I prefer a totally new theme. Tailwind UI would be awesome. Or I can go ahead and design a new theme.

Colors have to be fetched from config.toml. Colors would be the same as Bootstrap by default, but IMO we can add recommended color schemes to docs, maybe even for Bootstrap.

I'll continue working on Tailwind and "variants" if you agree with how the structure looks and how it's handled (except for colors not imported and postcss/tailwind config files), but let me know if you think a new theme should be developed or not.

@stp-ip
Copy link
Member

stp-ip commented Aug 5, 2020

Generally agree with the direction. Question is what do you define as a new "theme"?

@mpourismaiel
Copy link
Member Author

A new style in general for the Tailwind variant, if we decide not to go with Tailwind UI or add Tailwind UI as another variant. Tailwind can be used to implement any style, Bootstrap style as well. We can implement the entire Bootstrap look using Tailwind or we can take this opportunity to create a new style.

@stp-ip
Copy link
Member

stp-ip commented Aug 5, 2020

We could think of a new theme, but we also have to keep a check on the time spend as it's mainly duplicating functionality. I kinda like the thought of having a tailwindUI theme with all the things it enables or doesn't.

@stp-ip
Copy link
Member

stp-ip commented Aug 7, 2020

Needs a rebase due to the dep updates, but you wanted to finalize the general design ideas before merge anyway or?

@mpourismaiel
Copy link
Member Author

I'm gonna extract the variant code and create a pr for that and will work on design and create a few fragment using that. So this pr will be a point of reference and shouldn't be merged. Is that a good plan?

@stp-ip
Copy link
Member

stp-ip commented Aug 7, 2020 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants