midzer / tobii

An accessible, open-source lightbox with no dependencies
https://midzer.github.io/tobii/demo/
MIT License
196 stars 20 forks source link

Custom Properties Scoping #98

Open jakobhaerter opened 2 years ago

jakobhaerter commented 2 years ago

Naming of tobii's custom properties interferes with local custom properties due to generalized naming like --button-background or --button-color, set on :root level. A possible solution would be to scope variables inside tobii's parent node or to prefix everything, e.g. --tb-button-background.

Thanks!

midzer commented 2 years ago

Good idea, @jakobhaerter and thanks for filling!

As we have two possible solutions, which one would you prefer?

Prefixing those properties with --tobii-button-background might be the easier one. Are there any drawbacks?

viliusle commented 2 years ago

There is very very low risk breaking somebody code, but fixing it would give much more benefits.

midzer commented 2 years ago

@jakobhaerter @MoritzLost you want to help out and become a first time contributor for tobii?

MoritzLost commented 2 years ago

@midzer @jakobhaerter @viliusle Sure, I've opened a PR for this update here: https://github.com/midzer/tobii/pull/99

The prefix approach is simpler. Scoping the variables to .tobii would be a good idea as well, but there's two problems:

I think adding prefixes is completely sufficient. The PR adds those prefixes but prefers the old custom property names, if they still exist: var(--lightbox-background, var(--tobii-lightbox-background));

This way, the update is backwards compatible, so it can be a minor release. The fallback can then be dropped for the next major release.

By the way, I've noticed the project lacks a changelog, so I've added one in the PR. The changelog is a good place to include update instructions and deprecation notices like this one. @midzer Let me know if you want me to remove the changelog from the PR and/or add the update instructions and deprecation notice in some other place. If you want to make this a major update right away, we can also get rid of the fallback.

MoritzLost commented 2 years ago

@midzer Can you create a new release with the merged PR? The latest release v2.3.3 doesn't include it yet. Thanks!

midzer commented 2 years ago

https://github.com/midzer/tobii/releases/tag/v2.4.0 is out.

Let's leave this one open for an upcoming v3 when we can remove the backwards compatibility as a breaking change.

MoritzLost commented 1 year ago

@jakobhaerter This should probably be kept open as a reminder to drop the prefixes in 3.0.0 ;)

jakobhaerter commented 1 year ago

@MoritzLost @midzer Yeah, my bad! 🙏