-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
TINY-10978: Exported variables #9740
base: main
Are you sure you want to change the base?
Conversation
fd14974
to
255de0f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know CSS variables are supported by all of our browsers now, but I don't know if mixing and matching LESS with CSS variables is a good precedent.
There's no automated check to ensure the values are valid CSS, we might sneak a LESS function in there and it would need to be caught in QA.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turns out we already do some of this with comments. Maybe there's a migration discussion to be had; I would prefer CSS variables, but I don't want to sit on the fence with them and LESS. It would be one more thing for new developers to learn.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The less variables gets compiled to css so not sure how that would ever be raw less values unless the less compiler is broken or you use raw encoded strings. Either way we would need to have visual regression testing in general if we are afraid of things breaking. This becomes a new API and I think this is the way to go just because it's LESS right now doesn't mean it needs to be LESS in the future. We need someway to map our skin variables to uploadcare variables and this is the way to do it. The alternative would be more css file bloat and/or hardcode uploadcare specific stuff in the skin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't talking about raw values. What I meant was that CSS variables is something we've discussed migrating our entire UI to. They're a better solution, in my opinion, but they are a different solution.
Our approach in the past has been to put plugin CSS in the core so it can be skinned appropriately. I don't agree with that either, but we can't make up new ways to develop our product just because we disagree with how it's been done for years.
I'd rather schedule a complete swap from LESS to CSS variables in TinyMCE 8, and then we can finally start removing plugin CSS from the core.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't really be done if you use shadow dom unless we load the CSS into that shadow element that would either make it a new request to the entire skin or a separate file. Exporting variables seems like a simpler approach just export everything as variables then later when we actually switch to CSS variables then we could just use these exported properties as the values you can change. I ran into the same problem with the mathml preview I wanted to get the text color so that I could render white on black if the skin is a dark skin.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documented way of skinning uploadcare components is with CSS variables so somehow we need to get our variables to their variables this seems like the easiest way to do that. I guess in theory we could scope this to a uploadcare
class then compute the colors of that using these variable names if that is less dangerous.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The documented way of skinning uploadcare components is with CSS variables
Ok, all the talk so far has been building our own UI. That makes more sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We won't ever be building our own pickers that is to much work. We just skin what uploadcare provide. So the variables approach will stay for the release in one form or another. The custom UI will be for image manipulation.
255de0f
to
0bb30da
Compare
0bb30da
to
c8706b7
Compare
Related Ticket: TINY-10978
Description of Changes:
needed for the PR in the plugin
Pre-checks:
Changelog entry addedTests have been added (if applicable)feature/
,hotfix/
orspike/
Review:
Milestone setDocs ticket created (if applicable)GitHub issues (if applicable):