Skip to content

Conversation

innocenzi
Copy link
Contributor

This PR adds support for reactive CSS variables. I needed that because I have, on an app, multiple themes which I needed to get values from (in this case, a path for an image only readable on light background or dark background depending on the theme).

I thought it was a good idea to share the implementation on this awesome package, so I tried my best to do so even though I'm not used to contribute to packages this big.

The whole implementation relies on MutationObserver to observe changes on the DOM. When there are changes, the observed variables are updated.

const { stop, resume, get, set, listening, foreground } = useCssVariables({
  foreground: "color-foreground"
});
  • The stop method stops the observation
  • The resume methods resumes it
  • The get method is a helper to get a non-reactive CSS variable value directly from the given element
  • The set method updates the given CSS variable with the given value on the given element
  • The listening is a reactive boolean depending that is set to false when stop is called and true when resume is called
  • foreground or any other property from the given object is reactive

In this case foreground will update if --color-foreground is updated at the element level.

The default element is document.documentElement for everything.


Please note that the tests probably need some review - it's a first for me to perform these kind of tests and I'm sure there are either better ways to do, or some mistakes to fix. The tests are greatly inspired from this package's other tests.

The documentation on the other hand is updated and should work properly. I set up an example to demonstrate how useCssVariables work.

Also, the actual implementation is fully commented.

Copy link
Owner

@pikax pikax left a comment

Choose a reason for hiding this comment

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

Looks great, just some minor things :)

Thank you, the comments and documentation looks great :)

I would like to discuss the API of useCssVariables.

const css = useCssVariables({
  "--color-foreground": "yellow", // cssVar if starts with `--`
  "color-foreground": "red", // uses `--color-foreground`
  "bg-color": "blue", //
  foreground: {
    // cssVar name
    name: "color-foreground", // uses `--color-foreground`
    value: "red" // values
  },
  otherForeground: { 
    // cssVar name
    name: "--color-foreground", // uses `--color-foreground`
    value: "red" // values
  }
});

css['--color-foreground']; // 'ref<string>' == 'yellow'
css.foreground; // 'ref<string>' == 'red'

css.foreground.value = 'pink'; // calls set('--color-foreground', 'pink')

What do you think?

Comment on lines 122 to 124
attributes: true,
childList: true,
subtree: true
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
attributes: true,
childList: true,
subtree: true
attributes: true,
attributeFilter: ["style"]

We don't need to update if any child or subtree changes.

Copy link
Contributor Author

@innocenzi innocenzi Mar 1, 2020

Choose a reason for hiding this comment

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

After testing I think childList: true is actually needed, and that attributeFilter is not necessary.

In my case, for instance, I have a theme that is defined in my :root CSS. I also have a Logo component, deeper in the tree, that relies on a logo variable in my theme.

My theme is changed with the data-theme attribute on body: it means that if there is attributeFilter, this won't be listened to and my variable won't be updated. Also, without childList, the variable can't be fetched at all for a reason that I can't find yet.

I think it's not too specific to my case and that keeping only attributes: true and childList: true should cover most cases wihtout too much restraint.

Is it okay with you?

ef7eb87

Copy link
Owner

Choose a reason for hiding this comment

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

do you mind providing an example? I would be interested to see that case :)

*/
export function useCssVariables<T extends CssVariableConfigurationObject>(
variables: Record<keyof T, string>,
element: HTMLElement = document.documentElement
Copy link
Owner

Choose a reason for hiding this comment

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

This element should be RefTyped with a similar implementation to:

https://github.com/pikax/vue-composable/blob/master/packages/web/src/event/event.ts#L58-L82

The reason being you might want to change the cssVariable from a specific element, using composition-api you will use something similar to:

<template>
  <div  ref="myElement"/>
<template>
<script>
// ...
setup(){
  const myElement = ref(null);
  return useCssVariables(myVars, myElement); // my element is a ref null, only onMount will it hold a value
}
</script

Handling in a similar way as in the useEvent will allow the user to use v-if without handling the element change.

Let me know if you need some help here :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I tried to implement that, but I'm missing some knowledge about all of this. I don't understand yet how it is supposed to work, and what's happening behind the scenes. Also I won't be able to make tests for it because I don't know how to test with the templates yet. :/

Comment on lines 137 to 138
get: getVariableFor,
set: setVariableFor,
Copy link
Owner

Choose a reason for hiding this comment

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

get and set should be bound the the passed element

for example:

const {get} = useCssVariables({test: 'my-test'}, myElement);

get('my-test') // returns 'my-test' for document, instead of `myElement`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So if I understood correctly, myElement should be remembered by get and set when passed to useCssVariables?

I'm confused because your comment seems to say the inverse of what you said before (but I'm not a native speaker so I may be wrong).

Can you check and tell me if it's correct? 5afccf6

Copy link
Owner

@pikax pikax Mar 1, 2020

Choose a reason for hiding this comment

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

I'm confused because your comment seems to say the inverse of what you said before (but I'm not a native speaker so I may be wrong).

This is more as an usage, for example if you pass the element you expect the get to be bound to the element you passed. But in this case, calling get is the same as calling getVariableFor, meaning when a user calls get('my-var') it would be expected to getVariableFor to be called with:

getVariableFor('my-var', myElement);

Can you check and tell me if it's correct? 5afccf6

Yes, that's correct, I would even argue if allowing the element to be passed is useful, but is a nice touch :)

@pikax pikax merged commit f9049dd into pikax:master Mar 5, 2020
@innocenzi innocenzi deleted the feat/css-variables branch March 5, 2020 14:28
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.

2 participants