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: style attribute #4261

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

Conversation

m0ksem
Copy link
Contributor

@m0ksem m0ksem commented May 7, 2024

RAW PROPOSAL

closes #981

@m0ksem m0ksem requested a review from Fsss126 May 7, 2024 07:04
@asvae
Copy link
Contributor

asvae commented May 7, 2024

Is it meant to replace color prop or is it specifically for cases where you need more fine-grained control? (color often impacts background and text color).

@m0ksem m0ksem requested a review from asvae May 8, 2024 10:16
@m0ksem
Copy link
Contributor Author

m0ksem commented May 8, 2024

I'd like to keep current background/textColor behavior. But, what I see people struggle with is to understand what color actually stands for, so we can rename it to background in v2.

User must be able to use Component without global config. Right now, we require user to have global config registered even if only one simple component is used. It negatively affects bundle size.

For example, VaButton:

<VaButton />
:root {
  --va-button-background: var(--va-primary);
  --va-button-text-color: var(--va-on-primary);
}

.va-button {
   background: var(--va-button-background);
   color: var(--va-button-text-color);
}

We need to find a way to handle these styles with Global Config

Local CSS variables

Component changes to

<VaButton />
:root {
  --va-button-background: var(--va-primary);
  --va-button-text-color: var(--va-on-primary);
}

.va-button {
   background: var(--va-button-background-attr, --va-button-background);
   color: var( --va-button-text-color-attr, --va-button-on-background-attr, --va-button-text-color);
}

This means -attr suffix has priority over default CSS variables. We can set -attr variables with global config.
DOWNSIDES:

  • This essentially means we generate -on-background-attr even if it is not used.
  • CSS becomes much harder.
Global Config knows about CSS variables

We simply override component CSS variable.

<VaButton style:background="info" style:text-color="warning" />

Transformed to:

<VaButton :style="{ 
--va-button-background: getColor('info'), 
--va-button-text-color: getColor('warning') 
}" />

The problem appears when we don't have text-color attribute:

<VaButton style:background="info" />

Transformed to:

<VaButton :style="{ 
--va-button-background: getColor('info'), 
--va-button-text-color: getTextColor('info') 
}" />

How config can know how to set textColor? We'll need a script to handle this dependency. Similar to what we have now.

const { textColor } = useBackgroundAttr('background', 'textColor')
const useBackgroundAttr = (backgroundPropName: string, textColorPropName: string) => {
   if (attrs[textColorPropName]) {  return undefined }
   
   return getTextColor(attrs[backgroundPropName])
}

DOWNSIDES:

  • We need manually control CSS variables that can have color/textColor
Rename text-color to on-[property]
<VaButton />
:root {
  --va-button-background: var(--va-primary);
  --va-button-on-background: var(--va-on-primary);
}

.va-button {
   background: var(--va-button-background);
   color: var(--va-button-on-background);
}

We simply override component CSS variable.

<VaButton style:background="info" style:on-background="warning" />

Transformed to:

<VaButton :style="{ 
--va-button-background: getColor('info'), 
--va-button-on-background: getColor('warning') 
}" />

No problem if on-background is not passed. We can handle it globally.

<VaButton style:background="info" />

Transformed to:

<VaButton :style="{ 
--va-button-background: getColor('info'), 
--va-button-on-background: getTextColor('info') 
}" />

When generating variables in configTransform.

if (!isColor(prop)) { return  makeCssVarible(prop, attrs[prop]) }
if (hasOnColorProp(prop)) { 
	return [
	   makeCssVariable('on-' + prop, getTextColor(attrs[prop]),
	   makeCssVarible(prop, getColor(attrs[prop]))
	] 
}

return makeCssVarible(prop, getColor(attrs[prop]))

DOWNSIDE:

  • We make onColor even if we don't need text color. For example, errorMessageColor will generate onErrorMessageColor.

Looks like current behavior is the best.

@m0ksem
Copy link
Contributor Author

m0ksem commented May 8, 2024

Is it meant to replace color prop or is it specifically for cases where you need more fine-grained control? (color often impacts background and text color).

I'd like color prop to override --va-button-background in v1. In v2 we can remove color prop in favor of style: attributes.

Ideally, the initial idea was to have CSS variables without any prefix. <VaButton background="warning" /> But it means we need to store CSS variable names in JS - meaning bundle will store variable names two times: in JS and CSS (with default values). We can fix it by using default values in JS and generate it... But this makes it so much harder to handle for us. Prefix is a good solution to prevent larger bundle sizes. We'll still have TS types on attributes, there will no impact in runtime.

We also need to decide if <VaButton border-color="primary" /> is good, or we want this to be <VaButton style:border-color="primary" />

@m0ksem
Copy link
Contributor Author

m0ksem commented May 8, 2024

We also have :child prefix, which can be used to pass props/attributes to child components like:

<VaButton style:background="transparent" style:text-color="primary" child:loading-icon="{ 'style:color': 'success' }" />

This looks awful. But it is designed for presets, not direct usage:

myWeirdPresset: {
   'style:background': 'transparent',
   'style:text-color': (props) => props.color,
   'style:border-color': (props) => props.color,
   'child:loading-icon': {
       'style:color': 'success'
   }
}

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.

CSS variable changing
2 participants