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

Default theme will be loaded regardless of theme.config #736

Closed
Yahav opened this issue May 10, 2019 · 22 comments
Closed

Default theme will be loaded regardless of theme.config #736

Yahav opened this issue May 10, 2019 · 22 comments
Labels
lang/css Anything involving CSS type/bug Any issue which is a bug or PR which fixes a bug
Milestone

Comments

@Yahav
Copy link

Yahav commented May 10, 2019

Help Wanted

What i'm trying to do is have the FUI primary color be bounded to a css var(), the main issue is the usage of pre-processing functions.
So what i did is to copy the site.variables of the default them and copied it to /site/globals/site.variables and modified every primary related var to a css var(), like this:
@linkColor : var(--accent-link-color); @primaryBackground : var(--accent-bg-color);
I then went ahead and tried to compile and got the following:

{ [Error: error evaluating function darken: color.toHSL is not a function in file /home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables line no. 145] message: 'error evaluating function darken: color.toHSL is not a function in file /home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables line no. 145', stack: undefined, type: 'Runtime', filename: '/home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables', index: 3383, line: 145, column: 23, callLine: NaN, callExtract: undefined, extract: [ '@linkUnderline : none;', '@linkHoverColor : darken(saturate(@linkColor, 20), 15, relative);', '@linkHoverUnderline : @linkUnderline;' ], lineNumber: 145, fileName: '/home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables', name: 'Error', plugin: 'gulp-less', showProperties: true, showStack: false, __safety: { toString: [Function: bound ] } }

so i created a new theme folder, basically copied the default theme and renamed it, i then copied the /site/globals/site.variables file to this new theme's site.variables and changed every occurrence of 'default' at theme.config to the new theme's name.
and yet, i still get the same error which means FUI is trying to compile the themes/default/globals/site.variables file although i have defined the new theme AND have a site/globals/site.variables file overriding it a well.

I can simply overwrite the default theme site.variables and it will work (i had this same setup when i was using SUI) but it won't be wise.
Any ideas?

@lubber-de
Copy link
Member

@Yahav Thank you for the feedback

Indeed the button.less file had LESS color math functions implemented.
I prepared PR #737 which replaces those functions with proper variables instead, so you can change the desired values to CSS-Variable string in the central *.variables again as expected without worrying about the elements *.less files anymore.

You can directly try the change by replacing 3 files:

https://raw.githubusercontent.com/fomantic/Fomantic-UI/bf65cdee45a98a8e3040940eca13b34aa68647a3/src/definitions/elements/button.less
https://raw.githubusercontent.com/fomantic/Fomantic-UI/bf65cdee45a98a8e3040940eca13b34aa68647a3/src/themes/default/globals/colors.less
https://raw.githubusercontent.com/fomantic/Fomantic-UI/bf65cdee45a98a8e3040940eca13b34aa68647a3/src/themes/default/globals/site.variables

Would love if you give FUI a second chance again 😉

@exoego
Copy link
Contributor

exoego commented May 10, 2019

@Yahav

you totally ruined
you've damaged

Hey,that is not respectful way to communicate with people.
Even contributors, those who are dedicating themselves to maintain spirit of SUI, sometimes made a wrong design choice accidentally that may break some use case.
Contributors do not intend to damage SUI.

@lubber-de lubber-de added type/bug Any issue which is a bug or PR which fixes a bug lang/css Anything involving CSS labels May 10, 2019
@lubber-de lubber-de added this to the 2.7.x milestone May 10, 2019
@Yahav
Copy link
Author

Yahav commented May 10, 2019

@Yahav

you totally ruined
you've damaged

Hey,that is not respectful way to communicate with people.
Even contributors, those who are dedicating themselves to maintain spirit of SUI, sometimes made a wrong design choice accidentally that may break some use case.
Contributors do not intend to damage SUI.

You know what, you're quite right, imagine my high spirit when i found out SUI got a properly maintained fork, now imagine the heart break when i realized i won't be able to use it due to this.
But, you are absolutely right, this was written in the moment i realized i'm stuck with the unmaintined SUI.

@lubber-de Thank you very much, i'll have another look at it, i only hope it would work.

@Yahav
Copy link
Author

Yahav commented May 10, 2019

@lubber-de
I am stuck at the original intention of this post though, its probably not a bug but something i misunderstand. any advise?

@lubber-de
Copy link
Member

lubber-de commented May 10, 2019

@Yahav From what i understood and what i was able to reproduce :

  • You want to change all @primaryXXXX: something(@primaryYYY,??) or even @primaryXXXX: @primaryYYYY variables to somewhat like @primaryXXXX: var(--mycssvar)
  • You stumbled upon the LESS compiler error where a less function tried to calculate any given var(--mycssvar) string, which failed

Of course, you have to make sure, that (now only in your copied site.variables) all related @primaryXXXX variables get a fixed var(--something) value, because they still have lots of LESS functions applied (but now only at a central place).
If you miss any of them, you will still get the mentioned compiler error (but that should also happen in SUI...only FUI has a lot more additional variables because of new features)

If it seems to completely ignoring your whole site.variables then check, if you have adjusted the variable @site in theme.config to the foldername of your own theme.

@Yahav
Copy link
Author

Yahav commented May 10, 2019

@lubber-de
no, what i meant was, it won't be possible without editing themes/default/globals/site.variables
since it will compile the default theme site.variables even though another theme is specified at the theme.config file.
have a look a this:
Yahav/fomanticUiColorSystem@dea9dde

it still produces such errors:
[14:19:17] Created: dist/components/dimmer.js { [Error: error evaluating function darken: color.toHSL is not a function in file /home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables line no. 145] message: 'error evaluating function darken: color.toHSL is not a function in file /home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables line no. 145', stack: undefined, type: 'Runtime', filename: '/home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables', index: 3383, line: 145, column: 23, callLine: NaN, callExtract: undefined, extract: [ '@linkUnderline : none;', '@linkHoverColor : darken(saturate(@linkColor, 20), 15, relative);', '@linkHoverUnderline : @linkUnderline;' ], lineNumber: 145, fileName: '/home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables', name: 'Error', plugin: 'gulp-less', showProperties: true, showStack: false, __safety: { toString: [Function: bound ] } } { [Error: error evaluating function darken: color.toHSL is not a function in file /home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables line no. 145] message: 'error evaluating function darken: color.toHSL is not a function in file /home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables line no. 145', stack: undefined, type: 'Runtime', filename: '/home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables', index: 3383, line: 145, column: 23, callLine: NaN, callExtract: undefined, extract: [ '@linkUnderline : none;', '@linkHoverColor : darken(saturate(@linkColor, 20), 15, relative);', '@linkHoverUnderline : @linkUnderline;' ], lineNumber: 145, fileName: '/home/yahav/fomantic_themeColors/semantic/src/themes/default/globals/site.variables', name: 'Error', plugin: 'gulp-less', showProperties: true, showStack: false, __safety: { toString: [Function: bound ] } }

@lubber-de
Copy link
Member

lubber-de commented May 10, 2019

@Yahav
Looking at the first error (Line 145 of site.less)
-> @linkHoverColor

It seems LESS tries to immediatly recalculate @linkHoverColor when @linkColor is replaced by your own value without waiting until the whole file is parsed 🤔

As you are replacing the variables by something LESS cannot work with (using functions), i think there are only 2 options (didn't test):

1. Change order of variable definitions

LESS sees the current import order and tries to compile line by line immediatly:

//themes/default/globals/site.variables
@linkColor           : #4183C4;
@linkHoverColor      : darken(saturate(@linkColor, 20), 15, relative); //works

//themes/site/globals/site.variables
@linkColor           : var(--accent-link-color); // As @linkHoverColor is already defined at this time, less tries to recalculate it with the new @linkColor and breaks
@linkHoverColor      : var(--accent-link-hover-color);

Try to change the definition order to

//themes/default/globals/site.variables
@linkColor           : #4183C4;
@linkHoverColor      : darken(saturate(@linkColor, 20), 15, relative); //works

//themes/site/globals/site.variables
@linkHoverColor      : var(--accent-link-hover-color); //should work fine, because nowhere used for further calculation
@linkColor           : var(--accent-link-color); // should hopefully also work now, because , due to replacement of @linkHoverColor, it is not used anywhere else anymore

The same logic should be applied for every @primaryXXXX variable you changed.
Redefine the ones, which previously had a LESS function, at first (the first variable you replaced currently is @primaryColor which is used in several calculations before)

Try this order in your themes/site/globals/site.variables

@primaryColorHover        : var(--accent-hover-color);
@lightPrimaryColorHover   : var(--light-accent-hover-color);
@primaryColorFocus        : var(--accent-focus-color);
@lightPrimaryColorFocus   : var(--light-accent-focus-color);
@primaryColorDown        : var(--accent-color-darkened-10);
@lightPrimaryColorDown   : var(--light-accent-hover-color-darkened-10);
@primaryColorActive        : var(--accent-color-active);
@lightPrimaryColorActive   : var(--light-accent-color-active);

@primaryHeaderColor   : var(--accent-header-color);
@primaryRibbonShadow: var(--accent-color-darkened-10);
@primaryInvertedRibbonShadow: var(--light-accent-color-darkened-10);

@primaryColor        : var(--accent-color);
@lightPrimaryColor   : var(--light-accent-color);
@primaryBackground   : var(--accent-bg-color);
@primaryTextColor   : var(--accent-text-color);
@lightPrimaryTextColor   : var(--accent-light-text-color);

2. Make all your changes directly in the default site folder instead

If the above does not help, i fear the only solution will be to only use the default theme

@Yahav
Copy link
Author

Yahav commented May 10, 2019

@lubber-de
the preferred solution doesn't seem to work :(
it would be really unwise to override the default file, i'll continue looking for a solution, as css vars becomes widely used now days this would be a gigantic upgrade for the framework IMHO.

@Yahav
Copy link
Author

Yahav commented May 10, 2019

also, even when i override the default file, i still get this error:
{ [Error: error evaluating function each: property "tertiary" not found in file /home/yahav/fomantic_themeColors/semantic/src/definitions/elements/button.less line no. 1379] message: 'error evaluating function each: property "tertiary" not found in file /home/yahav/fomantic_themeColors/semantic/src/definitions/elements/button.less line no. 1379', stack: undefined, type: 'Name', filename: '/home/yahav/fomantic_themeColors/semantic/src/definitions/elements/button.less', index: 30516, line: 1379, column: 0, callLine: NaN, callExtract: undefined, extract: [ '', 'each(@colors, {', ' @color: replace(@key, \'@\', \'\');' ], lineNumber: 1379, fileName: '/home/yahav/fomantic_themeColors/semantic/src/definitions/elements/button.less', name: 'Error', plugin: 'gulp-less', showProperties: true, showStack: false, __safety: { toString: [Function: bound ] } }

@lubber-de
Copy link
Member

@Yahav Do you get the exact same error at the same file/line? Or is it different now (and did you implement those 3 files from my PR Fix ? Then there are additional @primaryTertiaryXXX variables now, which have to be considered!

as css vars becomes widely used now days this would be a gigantic upgrade for the framework IMHO.

Definately! But unfortunately the demand of keeping IE11 supported is too high atm, and CSS variables as well as pure ES6 is not supported by IE11 (and CSS variables cannot even get a reasonable polyfill)

@lubber-de
Copy link
Member

also, even when i override the default file, i still get this error:
{ [Error: error evaluating function each: property "tertiary" not found in file /home/yahav/fomantic_themeColors/semantic/src/definitions/elements/button.less line no. 1379] message: 'error evaluating function each: property "tertiary" not found in file /home/yahav/fomantic_themeColors/semantic/src/definitions/elements/button.less line no. 1379', stack: undefined, type: 'Name', filename: '/home/yahav/fomantic_themeColors/semantic/src/definitions/elements/button.less', index: 30516, line: 1379, column: 0, callLine: NaN, callExtract: undefined, extract: [ '', 'each(@colors, {', ' @color: replace(@key, \'@\', \'\');' ], lineNumber: 1379, fileName: '/home/yahav/fomantic_themeColors/semantic/src/definitions/elements/button.less', name: 'Error', plugin: 'gulp-less', showProperties: true, showStack: false, __safety: { toString: [Function: bound ] } }

Well, then my solution proposal seems to work! You only have to add the new tertiary variables from my PR to your site.variables accordingly! 😄

@lubber-de
Copy link
Member

lubber-de commented May 10, 2019

and you have to adjust your colors.less according to my PR (the missing "tertiary" keys are implemented there)

@Yahav
Copy link
Author

Yahav commented May 10, 2019

@lubber-de

@Yahav Do you get the exact same error at the same file/line? Or is it different now (and did you implement those 3 files from my PR Fix ? Then there are additional @primaryTertiaryXXX variables now, which have to be considered!

Same lines..or was i doing something wrong? hehe at some point it was getting pretty confusing.

Well, then my solution proposal seems to work! You only have to add the new tertiary variables from my PR to your site.variables accordingly!

already applied your PR, check this: https://github.com/Yahav/fomanticUiColorSystem/commits/master

@Yahav
Copy link
Author

Yahav commented May 10, 2019

Was missing those at the colors.less of the custom theme i created, once added it went ahead to the next error:
{ [Error: error evaluating function each: error evaluating function screen: Cannot read property '0' of undefined in file /home/yahav/fomantic_themeColors/semantic/src/definitions/collections/table.less line no. 619] message: 'error evaluating function each: error evaluating function screen: Cannot read property \'0\' of undefined in file /home/yahav/fomantic_themeColors/semantic/src/definitions/collections/table.less line no. 619', stack: undefined, type: 'Runtime', filename: '/home/yahav/fomantic_themeColors/semantic/src/definitions/collections/table.less', index: 14073, line: 619, column: 0, callLine: NaN, callExtract: undefined, extract: [ '', 'each(@colors, {', ' @color: replace(@key, \'@\', \'\');' ], lineNumber: 619, fileName: '/home/yahav/fomantic_themeColors/semantic/src/definitions/collections/table.less', name: 'Error', plugin: 'gulp-less', showProperties: true, showStack: false, __safety: { toString: [Function: bound ] } }
lines 644 and 660.

@lubber-de
Copy link
Member

lubber-de commented May 10, 2019

@Yahav I checked your code: As said, you have to apply the additional properties from
https://github.com/Yahav/fomanticUiColorSystem/blob/master/semantic/src/themes/default/globals/colors.less
also to
https://github.com/Yahav/fomanticUiColorSystem/blob/master/semantic/src/themes/radioio/globals/colors.less
The tertiary keys are missing there, thus resulting in the mentioned error message (because @Colors get completely overridden)

I would simply copy the file 😉 or leave it out/delete it, as long as the content is identical to the default one.

@Yahav
Copy link
Author

Yahav commented May 10, 2019

@lubber-de yeah, i figured it out, jesus this is quite confusing. anyway, check my previous message

@Yahav
Copy link
Author

Yahav commented May 10, 2019

@lubber-de What's wrong with ponyfill?
https://www.npmjs.com/package/css-vars-ponyfill
and as for pure ES6, you can just run it through babel, won't that solve the situation?

@lubber-de
Copy link
Member

lubber-de commented May 10, 2019

@lubber-de What's wrong with ponyfill?
https://www.npmjs.com/package/css-vars-ponyfill

Oh 👀 never came across this.
Nothing is wrong with polyfills, but it needs a clear decision to go and only use CSS Variables from a certain point (>= 3.x) .
That means we need to tell the users that we are only supporting IE11 via polyfills and not officially anymore then (so, its up to the polyfill maintainers to make everything work in IE11)

and as for pure ES6, you can just run it through babel, won't that solve the situation?

Yes, of course and this is planned for v3. But again, it's just IE11 which forces us to code in ES6, but provide ES5 (and its up to babel to make sure everything works as intended when coding in ES6)

But this is a separate discussion for V3, which we could continue in this thread instead

@Yahav
Copy link
Author

Yahav commented May 10, 2019

@lubber-de
from the PR:
No, this time the LESS function is part of a *.variable file (as all the other variables in site.variables as well) So in this case, as you modified the @linkColor variable in your theme, you also have to set the @invertedContentLinkColor to your own var(--) value, which you probably want anyway. (remember to set it before changing the @linkColor)

the order approach doesn't work (if i tried it correctly), i'm afraid the solution would be overwriting the default theme :(

@lubber-de
Copy link
Member

The problem in this case is, that the elements default variables are imported in any case right after the site.variables, so

//order of import
default/site.variables //(less functions)
custom/site.variables //(var(--) in "good" order
default/element.variables // it breaks, if here are again less functions refering to `var(--)` variables used😢 
custom/element.variables // (var(--) in "good" order ....but too late

Yes, i am afraid, at least for the default themes item.less, you should have to modify the @invertedContentLinkColor or move it to your sites.variables, whatever minifies the amount of work/maitainability for you.

@Yahav
Copy link
Author

Yahav commented May 11, 2019

@lubber-de
I haven't been able to make the:

custom/site.variables //(var(--) in "good" order

part as well, had to rewrite the entire default/site.variables.
There must be a way, otherwise, what good is a theme system.
updated my git to showcase the successful build when overriding the default theme as opposed to trying to use the theme system:
https://github.com/Yahav/fomanticUiColorSystem/

@lubber-de lubber-de modified the milestones: 2.7.x, 2.7.6 Jun 10, 2019
@lubber-de lubber-de added the tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build label Jun 10, 2019
@y0hami y0hami closed this as completed in e3d7eb6 Jun 16, 2019
@lubber-de lubber-de removed the tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build label Jun 16, 2019
@lubber-de
Copy link
Member

We fixed the default variable dependency issue now by #2857

@lubber-de lubber-de added the tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build label Jul 31, 2023
@lubber-de lubber-de removed the tag/next-release/nightly Any issue which has a corresponding PR which has been merged and is available in the nightly build label Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
lang/css Anything involving CSS type/bug Any issue which is a bug or PR which fixes a bug
Projects
None yet
Development

No branches or pull requests

3 participants