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

feature: support configure css variables generation location #3466

Merged
merged 9 commits into from
Jun 20, 2023

Conversation

lemoxcc
Copy link
Contributor

@lemoxcc lemoxcc commented Jun 2, 2023

support configure css variables generation location

Description

closes #3442
support configure css variables generation location, root element(html) or new style tag.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Improvement/refactoring (non-breaking change that doesn't add any feature but make things better)

Copy link
Collaborator

@m0ksem m0ksem left a comment

Choose a reason for hiding this comment

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

This is look great.

We also need to handle it in nuxt/packages. So instead of htmlAttrs we need to use content in useHead. Otherwise both style tag and html attribute will be rendered.

packages/ui/src/composables/useColors.ts Outdated Show resolved Hide resolved
@lemoxcc
Copy link
Contributor Author

lemoxcc commented Jun 6, 2023

This is look great.

We also need to handle it in nuxt/packages. So instead of htmlAttrs we need to use content in useHead. Otherwise both style tag and html attribute will be rendered.

No, html arribute is default. It just adds an optional configuration so that we can choose to transfer the color variables to a new style tag.
I don't think that I can decide where the color variables are generated (this decision should be done the vuestic-ui's owner). so I just add new optional configuration for those who need it.

@lemoxcc lemoxcc requested a review from m0ksem June 6, 2023 07:10
@lemoxcc
Copy link
Contributor Author

lemoxcc commented Jun 6, 2023

I got some screenshot for this.

  1. before, all color variables in the root (html) attributes.

Snipaste_2023-06-06_15-05-54

  1. when configuration styleTag is true. It's look tidy better.

Snipaste_2023-06-06_15-04-57

  1. and the color variable will be placed in the new generated style tag.

Snipaste_2023-06-06_15-05-25

@m0ksem
Copy link
Collaborator

m0ksem commented Jun 6, 2023

I added some changes related to nuxt, but there is still a small bug.

We render style tag in nuxt using useHead, but in vuestic-ui we render the same style tag using our utility. This is why we have two similar style tags in nuxt application. I think we should be able to prevent rendering any kind of style tags in ui, so it can be handled in nuxt (or another ssr solution).

Will make a different issue for that. Looks awesome for SPA btw. Thanks!

@m0ksem
Copy link
Collaborator

m0ksem commented Jun 6, 2023

@shiina7, would you like to write docs for this feature under Services/ColorConfig?

@lemoxcc
Copy link
Contributor Author

lemoxcc commented Jun 6, 2023

@shiina7, would you like to write docs for this feature under Services/ColorConfig?

@m0ksem sure, I'd love it! I will finish it in two days (before Thursday). Thank you for your additions for nuxt and merge request.

@m0ksem m0ksem requested a review from asvae June 8, 2023 09:12
@asvae
Copy link
Member

asvae commented Jun 15, 2023

@shiina7 does it make any point to keep previous solution and not just keep yours? (and get rid of styleTag)

@lemoxcc
Copy link
Contributor Author

lemoxcc commented Jun 16, 2023

@shiina7 does it make any point to keep previous solution and not just keep yours? (and get rid of styleTag)

@asvae my first point is to expend it, not change. beacuse I think that I can't decide it, but you(author) can. If you think it's okay to just keep the new style tag, I'm glad to change it.
But I think it's up to you to decide it. That's why added an option to configuration.

@lemoxcc
Copy link
Contributor Author

lemoxcc commented Jun 16, 2023

@shiina7 does it make any point to keep previous solution and not just keep yours? (and get rid of styleTag)

@asvae my first point is to expend it, not change. beacuse I think that I can't decide it, but you(author) can. If you think it's okay to just keep the new style tag, I'm glad to change it. But I think it's up to you to decide it. That's why added an option to configuration.

Perhap someone needs this option and we can keep, If we want keep new tag we can set the styleTag to true by default. BTW

@m0ksem
Copy link
Collaborator

m0ksem commented Jun 19, 2023

I think we can remove previous solution. It was made asap and not documented, so we can just drop it.

@lemoxcc
Copy link
Contributor Author

lemoxcc commented Jun 19, 2023

I think we can remove previous solution. It was made asap and not documented, so we can just drop it.

Okay, got it. It's will be change it tomorrow.

@lemoxcc
Copy link
Contributor Author

lemoxcc commented Jun 20, 2023

I think we can remove previous solution. It was made asap and not documented, so we can just drop it.

Okay, got it. It's will be change it tomorrow.

@m0ksem @asvae I removed the styleTag option and only support generated css variables to style tag now. review it please, maybe it can be included in the next version.

@asvae
Copy link
Member

asvae commented Jun 20, 2023

It's actually great to have in context of 1.7.0, as if something would break - users would probably somewhat expect it :D.

Thanks @shiina7 🤗

@asvae asvae merged commit ad5ad2f into epicmaxco:develop Jun 20, 2023
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.

Remove CSS variable from root(html element)
3 participants