abishekatp / stylers

Fully compile time scoped CSS for Leptos components
MIT License
139 stars 13 forks source link

refactor: fix clippy warnings and use native cargo tests #33

Closed mrnossiom closed 1 year ago

mrnossiom commented 1 year ago

Hi πŸ‘‹πŸ»

A quick list of the changes I've made:

What I want to do:

@abishekatp Tell me if you don't like some of the changes I've made

mrnossiom commented 1 year ago

Is there a specific question classes are prefixed with l- or is this a random choice ?

abishekatp commented 1 year ago

@MrNossiom Thank you for contributing. I have also wanted to refactor the code for long time now and tried different approach in another branch. What you have done seems better than that. I will test these changes locally once and merge it.

mrnossiom commented 1 year ago

I would love to see stylers being nightly-free, I know having access to features is great, but we could find a workaround, maybe under a flag... Anyway, I didn't understand fully what does the add_spaces functions does? Can you enlighten me?

mrnossiom commented 1 year ago

I am also going to clean examples, leaving only 3: demo (all stylers macros), trunk, leptos

abishekatp commented 1 year ago

I am also going to clean examples, leaving only 3: demo (all stylers macros), trunk, leptos

Yeah that would be nice.

abishekatp commented 1 year ago

I would love to see stylers being nightly-free, I know having access to features is great, but we could find a workaround, maybe under a flag... Anyway, I didn't understand fully what does the add_spaces functions does? Can you enlighten me?

First of all @MrNossiom I think you have done great work. I was going through your changes and it makes sense.

So about what you asked, We know that in the css selectors whitespace information is an important thing. This add_spaces function will check if there is some whitespace between current rust token and previous rust token using the span information and update the pre_line and pre_col variables for next check.

The another change that would be useful:

Note: This let start = span.unwrap().start(); uses the official proced_macro crate. Because of this we have to declare this crate as proc_macro crate in the Cargo.toml file. This creates a limitation that we can only export functionalities of this crate as macros.

Solution I thought of: We can extract both the style and style_sheet modules to the separate crate and just use them in the style macro crate.

@MrNossiom What do you think about this?

abishekatp commented 1 year ago

Currently I am not able to find some way of getting the span information so I am using this approach. If you think of some other way to retain the whitespace information of the css content then we can optimize this library like we discussed in this issue.

abishekatp commented 1 year ago

@MrNossiom Hi Sorry for taking so long. I have been working on fixing the above mentioned issue for some time. The good news finally I was able achieve it. But the bad news is those changes makes the rust workspace neccessary. So I don't think we will be able to merge this request directly. Will you be able to make these changes you have done here in the new implementation branch Or should I do it? If you are willing to make the changes in another branch please give me one or two days to finish that implementation.

mrnossiom commented 1 year ago

Hi @abishekatp, I may be less responsive since I'm on vacation but I would suggest that merging your new branch into mine would be simple. I can take care of resolving the conflicts. The changes I've made are already mergeable, I'll do another PR to complete this one.

abishekatp commented 1 year ago

Okan then I will merge that stylers_build branch with the main branch then you can resolve all the merge conflicts here. But please look into other branch changes if you get some time there I have extracted the css parsing logic from the proc_macro crate so that we can reuse that logic for both proc_macro and other use cases. Thank yoy. Enjoy the vacation:)

mrnossiom commented 1 year ago

I would have first merged mine, or merged your changes into this branch but you choose.

abishekatp commented 1 year ago

We can also merge your request but we have to keep the workspace structure of the project. Because it is needed by the new implementation where I have extracted stylers core logic into a separate crate and stylers macro as a separate crate. Can you make those changes in this branch?

mrnossiom commented 1 year ago

Done!

abishekatp commented 1 year ago

It looks good @MrNossiom I will merge it in this week.

abishekatp commented 1 year ago

Hi @MrNossiom sorry for disturbing again, I am trying to run these test cases using cargo test but it doesn't seem running test cases. Is there a specific way to run them?

mrnossiom commented 1 year ago

You were right, I forgot to move the tests when going back to a workspace.

abishekatp commented 1 year ago

You were right, I forgot to move the tests when going back to a workspace.

Yeah okay

mrnossiom commented 1 year ago

Thanks a lot πŸ₯³

abishekatp commented 1 year ago

Thanks a lot πŸ₯³

Actually Thanks to you @MrNossiom for your contribution(pardon me for taking this much time though πŸ˜‚).