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

fix(theming): add custom theme support for external CSS #5887

Merged
merged 2 commits into from
Oct 28, 2022
Merged

fix(theming): add custom theme support for external CSS #5887

merged 2 commits into from
Oct 28, 2022

Conversation

nnaydenow
Copy link
Contributor

@nnaydenow nnaydenow commented Oct 5, 2022

Now, we support two scenarios where OpenUI5/ SAPUI5 is detected and stand-alone web components project.

If OpenUI5Support is feature is registered, we leave previous implementation where sap.ui.core CSS variables should be imported (data-sap-ui-xx-cssVariables should be used in that scenario). CSS variables will be loaded from url + UI5/sap/ui/core/themes/custom_theme/css_variables.css

If OpenUI5Support feature is not registered we load custom CSS variables and rerender all components. CSS variables will be loaded from url + Base/baseLib/custom_theme/css_variables.css

Like in OpenUI5 when CSS variables are loaded from different origin than current, meta tag with allowed theme roots should be used (sap-allowedThemeOrigins).

  • Checks for theme origin
  • Should we extend the theme origin with UI5?

Copy link
Contributor

@vladitasev vladitasev left a comment

Choose a reason for hiding this comment

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

The proposed solution only solves the "theming" package CSS Variables ( the official ones @sapSomething), however we must also load the correct geometric variables for the packages such as @ui5/webcomponents and @ui5/webcomponents-fiori, depending on the base theme, upon which the custom theme passed with the @ syntax was crated in Theme designer.

So the full solution would be something like this: once the custom theme has loaded, use the functions in packages/base/src/theming/getThemeDesignerTheme.js to detect the base theme upon which the custom theme was built, and then trigger loading the CSS Vars for all registered packages.

Example: my custom @-syntax theme loads, then the framework determines it was based on sap_horizon, and for all currently loaded packages, also load the CSS vars for this base theme.

@@ -40,6 +41,7 @@ const boot = async () => {
F6Navigation.init();
}

attachStylesToHead();
Copy link
Contributor

Choose a reason for hiding this comment

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

IMO boot.js should not have knowledge of this functionality, if possible.

Instead, I would suggest to hook this attachStylesToHead code to the applyTheme function, which is called a few lines below. If you do this, this will also automatically trigger when calling setTheme, as it calls applyTheme internally, so you won't need to change that function either.

packages/base/src/config/ThemeRoots.js Outdated Show resolved Hide resolved
packages/base/src/config/ThemeRoots.js Outdated Show resolved Hide resolved
packages/base/src/config/ThemeRoots.js Outdated Show resolved Hide resolved
packages/base/src/util/createLinkInHead.js Outdated Show resolved Hide resolved
packages/base/src/util/removeLinkFromHead.js Outdated Show resolved Hide resolved
@nnaydenow nnaydenow requested a review from vladitasev October 10, 2022 13:18
@nnaydenow nnaydenow requested a review from vladitasev October 27, 2022 21:08
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