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

Using dynamic classes #117

Closed
tedmeftah opened this issue Apr 13, 2021 · 8 comments
Closed

Using dynamic classes #117

tedmeftah opened this issue Apr 13, 2021 · 8 comments
Assignees
Labels
question Further information is requested

Comments

@tedmeftah
Copy link
Contributor

After finding out that using windi from windicss/helpers did not work in dev mode (windicss/windicss#244). I noticed that in the code of this package there is a check for the for the windi template:

// Match windi template syntax
let WINDI_EXPRESSION = lines[i].toString().match(/windi\`(.*?)\`/i);
if (WINDI_EXPRESSION) {
const INTERPRETED_WINDI_EXPRESSION = PROCESSOR.interpret(WINDI_EXPRESSION[1]);

so we don't actually need to import the windi helper we can just fake it like this:

function windi(parts, ...vars) {
  return parts.reduce((out, part, i) => (out += part + (vars[i] || '')), '')
}

which make this work

<div class={`bg-red-50`}>This does not work since it's an expression</div> 

<div class={windi`bg-red-100`}>This works</div>

of course this is not so useful, since the helper does update the classes in runtime, so all these examples below don't work:

<script>
  function windi(parts, ...vars) {
    return parts.reduce((out, part, i) => (out += part + (vars[i] || '')), '')
  }

  let shade = 300
  let dynamicClass = windi`bg-red-${shade}`
  let toggle = false
</script>

<button on:click={() => toggle = !toggle}>Toggle [{toggle}]</button>

<div class={windi`bg-red-${shade}`}>My background is dynamic</div>

<div class={dynamicClass}>My background is dynamic</div>

<div class={windi`${toggle ? 'bg-blue-500' : 'bg-red-500'}`}>My background is togglable</div>

I feel that the last example should work but it does not (class={windi`${toggle ? 'bg-blue-500' : 'bg-red-500'}`}).

what's weird is when I do this:

<div class={toggle ? windi`bg-yellow-500` : windi`bg-blue-500`}>My background is togglable</div>

it partially works, only taking the first windi expression (windibg-yellow-500) but it toggle just fine.

I found the best way to do dynamic styles is to use custom classes and @apply,

<script>
  let toggle = true
</script>

<button on:click={() => toggle = !toggle}>Toggle [{toggle}]</button>
<div class={toggle ? "red" : "blue"}>My background is togglable</div>

<style>
  .red {
    @apply bg-red-800;
  }
  .blue {
    @apply bg-blue-800;
  }
</style>

I think we should advocate this way to use dynamic classes in the docs, since this avoids adding code to the runtime.

An improvement that can be added to this is allow the mixing of windicss classes, because this does not work right now:

<script>
  function windi(parts, ...vars) {
    return parts.reduce((out, part, i) => (out += part + (vars[i] || '')), '')
  }
  
  let toggle = true
</script>

<button on:click={() => toggle = !toggle}>Toggle [{toggle}]</button>

<div class={toggle ? "red text-4xl" : "blue text-6xl"}>My background is togglable</div>

<div class={toggle ? windi`red text-4xl` : windi`blue text-6xl`}>My background is togglable</div>

<style>
  .red {
    @apply bg-red-800;
  }
  .blue {
    @apply bg-blue-800;
  }
</style>

it only render the red and blue classes, and like before only render windired text-4xl` in the second example.

@alexanderniebuhr alexanderniebuhr self-assigned this Apr 13, 2021
@alexanderniebuhr alexanderniebuhr added the question Further information is requested label Apr 13, 2021
@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 13, 2021

TLDR: at all I think you are using windi`` to often

for dynamic classes on variable, the svelte way would be

<div class:bg-red-800={toggle} class:bg-blue-800={toggle}>My background is togglable</div>

so we don't actually need to import the windi helper we can just fake it like this

you need to import it, since windi`` is used to interpret classes in runtime, the code in the pre-processor just makes sure it does add that classes to safelist


<div class={`bg-red-50`}>This does not work since it's an expression</div> 

that should work, maybe use ' or "


<div class={windi`${toggle ? 'bg-blue-500' : 'bg-red-500'}`}>My background is togglable</div>

just use

<div class={toggle ? 'bg-blue-500' : 'bg-red-500'}>My background is togglable</div>

<div class={toggle ? "red text-4xl" : "blue text-6xl"}>My background is togglable</div>

we would need debug + verbosity 3, 4 & 5 for this bug

@tedmeftah
Copy link
Contributor Author

Thank you @alexanderniebuhr, I agree that's too much use for windi but they were just examples to test all the possible cases.

I could not import windi since it has an issue while running yarn dev (windicss/windicss#244). and since I want to avoid adding code to the runtime I will try to avoid it.

I did more dinging in the code and I noticed that this does not work:

<div class={toggle ? windi`bg-yellow-500` : windi`bg-blue-500`}>My background is togglable</div>

because it seems that we are only taking the first match of windi expression per line:

const INTERPRETED_WINDI_EXPRESSION = PROCESSOR.interpret(WINDI_EXPRESSION[1]);

and for this:

<div class={toggle ? "red text-4xl" : "blue text-6xl"}>My background is togglable</div>

it looks like we do format the code first, then we only match classes like class="..." or class='...' since ' get converted to " when formatted using prettier.

This ignores class=`...` and class={"..."} which are valid classes and should be checked too.

This is the current regex:

[DEBUG] regex parse matcher ((class|w:sm|w:-sm|w:+sm|w:md|w:-md|w:+md|w:lg|w:-lg|w:+lg|w:xl|w:-xl|w:+xl|w:hover|w:focus|w:visited|w:checked)=["])([^"]*)(["])

@alexanderniebuhr
Copy link
Member

alexanderniebuhr commented Apr 13, 2021

<div class={toggle ? windi`bg-yellow-500` : windi`bg-blue-500`}>My background is togglable</div>

I don't think this would be good way of writing this, so not sure if we make it work. I think to use windi`` i would suggest doing

<script>
let myDynamicToggleClasses = windi`bg-yellow-500`
</script>
<div class={toggle ? myDynamicToogleClassses}>My background is togglable</div>

<div class={toggle ? "red text-4xl" : "blue text-6xl"}>My background is togglable</div

yeah i will fix this right now.. seems that there are some bugs

@tedmeftah
Copy link
Contributor Author

I agree it's not a good way to write the code like that but it was just to test since it's valid syntax. The issue is that only the first windi expression in a line gets parsed:

<script>
  let toggle = true

  let dynamicClassA = windi`bg-yellow-500`

  let dynamicClassB = windi`bg-red-500`

  $: dynamicClassFinal = toggle ? dynamicClassA : dynamicClassB

  $: dynamicClass = toggle ? windi`bg-blue-500` : windi`bg-green-500`
  // for completion this does not work either 
  // $: dynamicClass =  windi`${toggle ? "bg-blue-500" : "bg-green-500"}`

  let [dynamicClassC, dynamicClassD]= [windi`bg-pink-500`, windi`bg-orange-500`]
</script>

<button on:click={() => toggle = !toggle}>Toggle [{toggle}]</button>

<div class={toggle ? dynamicClassA : dynamicClassB}>This works fine</div>

<div class={dynamicClassFinal}>This works fine</div>

<div class={dynamicClass}>only the first class is taken</div>

<div class={toggle ? dynamicClassC : dynamicClassD}>only the first class is taken</div>

Again I'm not saying either of those is good way to write, but it's valid syntax and someone else may see a similar issue so it's worth documenting.


Thank you again @alexanderniebuhr for the quick response I would like to help more but I'm still new to OSS, and still trying to understand the codebase better. hopefully I'll be able to contribute more soon 😃

@alexanderniebuhr
Copy link
Member

@tedmeftah do not try to understand the codebase now.. check branch next
If you would like to help, I think the best currently (as you did above, is give me a list of test cases)

@alexanderniebuhr
Copy link
Member

@tedmeftah you said, you would like to contribute. if you have time, feel free to open PR onto the next branch, where you add .svelte files with test cases in this directory. filename should be descriptive and it should be one test case per file only.
https://github.com/windicss/svelte-windicss-preprocess/tree/next/tests/assets/input

@alexanderniebuhr
Copy link
Member

still tracking by v4.1

@alexanderniebuhr
Copy link
Member

should be fixed with 7482f9a

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested
Projects
None yet
Development

No branches or pull requests

2 participants