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.

Sign up for GitHub

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: custom theme detection #10410

Merged
merged 3 commits into from
Dec 19, 2024
Merged

fix: custom theme detection #10410

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a5fc7c3
fix: custom theme detection
vladitasev Dec 18, 2024
03e89a3
chore: add fallback
vladitasev Dec 18, 2024
b7bd9bc
chore: lint
vladitasev Dec 18, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion packages/base/src/theming/getThemeDesignerTheme.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ const processThemeMetadata = (metadata: ThemeMetadata): ThemeDescriptor | undefi
let baseThemeName;

try {
themeName = metadata.Path.match(/\.([^.]+)\.css_variables$/)![1];
themeName = metadata.Path.split(".")![2];
Copy link
Contributor

Choose a reason for hiding this comment

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

the ! will eventually happen to be undefined, and we are back in the scenario where an exception is thrown and no theme is loaded. Can we provide a better fallback?

Also, this was the case before, but what if a custom theme has a . in the name? is it escaped? should we also check this?

Copy link

Choose a reason for hiding this comment

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

Custom themes can't have a . in the theme name, otherwise they couldn't even be saved by the theming engine.

Starting with SAP/theming-base-content@11.23.0, there is --sapSapThemeId, which might be a good fallback (is always an SAP theme, but that might be what you're after anyways):

themeName = getComputedStyle(document.body).getPropertyValue('--sapSapThemeId');
// is "sap_horizon" for SAP Morning Horizon,
// is "sap_horizon" for custom themes based on SAP Morning Horizon

Maybe you don't even need JavaScript for that anymore, because you can target --sapSapThemeId with CSS container style queries – see Theme-specific CSS for your application.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the insight!

I changed the check a bit - if there are 4 parts (after splitting) then this means we're with the "known" format and themeId is guaranteed to be the third part.

However, if this is not the case (future versions of the metadata), we read the CSS var as this is the standard as of last release.

baseThemeName = metadata.Extends[0];
} catch (ex) {
if (!warnings.has("object")) {
Expand Down
Loading