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

review: align with tailwind #124

Closed
11 tasks done
userquin opened this issue Nov 18, 2021 · 18 comments
Closed
11 tasks done

review: align with tailwind #124

userquin opened this issue Nov 18, 2021 · 18 comments

Comments

@userquin
Copy link
Member

userquin commented Nov 18, 2021

@userquin
Copy link
Member Author

@antfu @hannoeru @edwardnyc

On weekend I'll do a second review.

@hannoeru
Copy link
Collaborator

hannoeru commented Nov 19, 2021

Just tried the convert to issue function 😂, can I handle first one?

@userquin
Copy link
Member Author

Ofc Man, I added the task for it... you can also asign yourself the issue

@userquin
Copy link
Member Author

userquin commented Nov 19, 2021

@hannoeru I assign it to you, see assignees from right panel on the issue, we need to check if the title can be changed or modify in some way, it is huge.

Edit: just change the issue title.

@xiaojieajie
Copy link
Contributor

Hello, I want to help solve the seventh task( object-center missing rule: object-position: center;, we should add the variants (top, right...))

@hannoeru
Copy link
Collaborator

@xiaojieajie Click convert to issue button on right side of list to create issue and assign yourself, then you are good to go!

@xiaojieajie
Copy link
Contributor

@xiaojieajie单击列表右侧的转换为问题按钮以创建问题并分配给自己,然后就可以开始了!

Ask me, I created issue, but I don't think the task can be selected?

@zyyv
Copy link
Member

zyyv commented Nov 19, 2021

Can i handle the last one?

@hannoeru
Copy link
Collaborator

@xiaojieajie Task will auto checked when you close that issue.

@hannoeru
Copy link
Collaborator

Can i handle the last one?

Of course.

@userquin
Copy link
Member Author

wtf: I created the issue this very morning, I just woke up and almost all the tasks started / closed, you are all amazing.

Thank you very much to all.

@UltraCakeBakery
Copy link
Contributor

Can you add translate-x-full, translate-y-full and their negative variants to the list? @userquin

@userquin
Copy link
Member Author

@UltraCakeBakery added and created issue

@zojize
Copy link
Contributor

zojize commented Nov 29, 2021

::before and ::after variants do not have content: '' by default. This is misaligned with tailwind according to this link and makes setting pseudo-elements practically ineffective. And there are no rules for content-* to set this content property. Another misalignment I found is space insertion/replacement when matching against arbitrary values, rules like h-[calc(1000px-4rem)] will generate invalid CSS since spaces are not inserted in the correct places. Can we add support to these?

Also, I tried creating my own content-* rule as follows:

[/^content-\[(.*)\]$/, ([, content]) => ({ content })],

However, this is unable to match rules containing quotes such as content-['']. The regexp should be fine, did I miss out on something here?

@antfu
Copy link
Member

antfu commented Nov 29, 2021

::before and ::after variants do not have content: '' by default

I am not sure why TW does this, won't it affects overall client performance since now every element comes with two pseudo-elements?

And there are no rules for content-* to set this content property

I am personally not a fan of this utility as I saw it kinda a mess to allow user provide arbitrary content in class, I feel that using inline style is more approach in this case.

h-[calc(1000px-4rem)] will generate invalid CSS since spaces are not inserted in the correct places

That's a good point, added to the list.

content-['']

Quotes are separators when we do the extracting. You can use JSON.strinify to escape the value:

[/^content-\[(.*)\]$/, ([, content]) => ({ content: JSON.stringify(content) })],

and use it like content-[]

@zojize
Copy link
Contributor

zojize commented Nov 29, 2021

First, thanks for your kind reply!

I am not sure why TW does this, won't it affects overall client performance since now every element comes with two pseudo-elements.

If I remember correctly, ::before and ::after pseudo-elements won't render if no content property is provided. Which I think is why Tailwinds sets content: '' by default.

I am personally not a fan of this utility as I saw it kinda a mess to allow user provide arbitrary content in class, I feel that using inline style is more approach in this case.

And this is the one and only case where I find this content-* to be useful, that is used along with a ::before/::after variant like so. (e.g. before:content-[''], before:content-[counter(myCounter)])

Quotes are separators when we do the extracting. You can use JSON.strinify to escape the value:

Hmm, not too big of a fan of having quotes as separators. I'm thinking if it's possible for user-defined custom separators as a field in our configuration? Something along the lines of separators: [/\s/]. Just a wild proposal.

@antfu
Copy link
Member

antfu commented Nov 29, 2021

If I remember correctly, ::before and ::after pseudo-elements won't render if no content property is provided.

Yes. But applying to all elements just because that someone might need it does not sound good to me. I kinda prefer to introduce a new content-empty util that you can explicitly set like class="before:content-empty before:p-4"

I'm thinking if it's possible for user-defined custom separators as a field in our configuration

Yes, you can provide your custom extractor. https://github.com/antfu/unocss/blob/main/packages/core/src/extractors/split.ts

Something along the lines of separators: [/\s/]

That wouldn't work actually. Consider you have class="p-4 m-5" it will be split into class="p-4 and m-5". Not saying it's impossible to do right, just feel it's not worth it to sacrifice performance and maintainability for a relatively rare use-case (but you can always provide your custom one for sure)

chu121su12 added a commit to chu121su12/unocss that referenced this issue Dec 1, 2021
`.text-[unocss#124]` is producing `.text-[unocss#124]{font-size:unocss#124;line-height:1;}` in addition to the color + opacity rule
@userquin
Copy link
Member Author

epic

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

No branches or pull requests

7 participants