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

Custom Properties Scoping #98

Open
jakobhaerter opened this issue Apr 19, 2022 · 8 comments
Open

Custom Properties Scoping #98

jakobhaerter opened this issue Apr 19, 2022 · 8 comments
Labels
enhancement New feature or request good first issue Good for newcomers
Milestone

Comments

@jakobhaerter
Copy link

jakobhaerter commented Apr 19, 2022

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 midzer added the enhancement New feature or request label Apr 22, 2022
@midzer
Copy link
Owner

midzer commented Apr 22, 2022

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
Copy link
Collaborator

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

@midzer
Copy link
Owner

midzer commented Apr 22, 2022

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

@midzer midzer added the good first issue Good for newcomers label Apr 22, 2022
@MoritzLost
Copy link
Contributor

@midzer @jakobhaerter @viliusle Sure, I've opened a PR for this update here: #99

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

  • Tobii also uses custom properties for some other classes, like tobii-zoom, so the properties would need to be added to all of those.
  • Changing the scope to those classes would result in a BC break, because then they would overwrite custom properties defined by users in the :root scope.

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
Copy link
Contributor

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

@midzer
Copy link
Owner

midzer commented Jun 23, 2022

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.

@midzer midzer added this to the v3.0.0 milestone Jun 23, 2022
@MoritzLost
Copy link
Contributor

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

@midzer midzer reopened this Apr 19, 2023
@jakobhaerter
Copy link
Author

@MoritzLost @midzer Yeah, my bad! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants