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

feat: Support default values for v-bind in CSS #11160

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

leemotive
Copy link

The default value is defined by an SCSS variable. If the JavaScript variable is null, I want to use the SCSS variable as the fallback.

I don't want to use :export and import in all components

// variable.scss in library
$theme-color: blue !default;   // can be overridden in the project if needed
<script setup>
defineProps({
  color: {
    color: String,
    default: null,
  }
})
</script>
<style>
.mm {
  color: v-bind(color, $theme-color)
}
</style>

wiill be transfromed to var(--var-name, $theme-color)

--var-name will be a hashed name, and $theme-color will hold the value assigned to $theme-color.

@@ -103,8 +103,8 @@ describe('CSS vars injection', () => {
const { content } = compileSFCScript(
`<script>const a = 1</script>\n` +
`<style>div{
color: v-bind(color);
font-size: v-bind('font.size');
color: v-bind(color, blue);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it might be better to add new tests for this feature, rather than modifying the existing tests. Otherwise we lose tests for the case where a default isn't provided.

Copy link
Author

Choose a reason for hiding this comment

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

I have already re-modified and rebased my own branch

@skirtles-code
Copy link
Contributor

There hasn't been an RFC for this change. While I understand that it is a relatively small change, I do worry that we may miss out on feedback from the community without an RFC.

I read through the original RFC for v-bind in <style>. The use case described here doesn't seem to be discussed, but I did find two comments that seemed relevant:

Another comment notes a potential workaround:

--myColor: v-bind('myReactiveColor');
color: var(--myColor, black);

This does appear to work (Playground) and I believe it would be compatible with pre-processors.


As for the implementation in this PR, I worry about backwards compatibility. Commas could already appear in expressions for other reasons, e.g.:

Of course the code can be fixed, but I think it illustrates that 'working' code could potentially be broken by this change.

@leemotive
Copy link
Author

@skirtles-code
Thank you for your response and suggestions. I hadn't used v-bind like v-bind(['red', 'blue'][value]) before; I had always used the quoted form v-bind('[red, blue][value]'), so I didn't consider the case with the comma in the expression. I still believe that having v-bind support default values would be beneficial, so I have resubmitted the code. If the maintenance team feels that it's not suitable to add this feature at this time, You can close this PR or I will do it.

@edison1105 edison1105 added need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. ✨ feature request New feature or request labels Aug 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need guidance The approach/solution in the PR is unclear and requires guidance from maintainer to proceed further. ✨ feature request New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants