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: site theme not initialized correctly from storage #19170

Merged
merged 23 commits into from
Sep 6, 2024

Conversation

Platonn
Copy link
Contributor

@Platonn Platonn commented Aug 28, 2024

BEFORE:

  • theme value was not restored correctly from local storage on page start
  • we performed a mandatory call to OCC endpoint /basesites, even if customers didn't want to use CMS-driven theming
Elaborating more:

When we attempted to set a value from storage on page start, we encountered an ASYNC mechanism of validation (which in the background called the OCC endpoint /basesites). Unfortunately, then we immediately expected the value to have been set (despite it was delayed by an async validation) - we checked synchronously the value of the isInitialized(). This race condition led to setting improper value to ngrx on the page start.

Besides this problem, we also forced all customers to make in an APP_INITIALIZER an obligatory render-blocking http call to OCC endpount /basesites, (even regardless if they used CMS-driven theming or not). Please note that we already had a different generic mechanism (SiteContextConfigInitializer) which performs such a render-blocking call, but customers only if customers decide to load site context data from CMS (alternatively they can configure site context statically in Spartacus config). It's a tradeoff each customer need to make - to have dynamic CMS-driven site context, Spartacus needs to make a http roundtrip.

AFTER:

  • theme value is restored correctly from storage on page start
  • we let the existing mechanism SiteContextConfigInitializer to possibly fetch the theme from CMS - only if a customer decides to load site context data from CMS. Alternatively, a customer can configure the theme statically in Spartacus config config.context.

Side changes that needed to be made:

  1. Reverted the deprecation on the confi config.context[THEME_CONTEXT_ID]: string
  2. Renamed the new config config.siteTheme.siteThemes to config.siteTheme.optionalThemes.
  3. The themes are back again being configured via scattered 2 different configs (Note: it was the case at first in the main PR, until I first suggested to unify those configs for simplicity; only now I realized negative consequences unifying them, so reverting this decision), so now:
    • the config config.context[THEME_CONTEXT_ID]: string defines the default theme ID (which can be configured manually in Spartacus config OR it can be populated dynamically by the SiteContextConfigInitializer if a customers decides so. So we preserve the old behavior.
    • now the new config config.siteTheme.optionalThemes: SiteTheme[] defines ONLY the optional themes (like high contrast ones), with their IDs (class names) and a11y labels
  4. If the default theme ID defined in config.context[THEME_CONTEXT_ID]: string is left undefined, we use an empty string '' as the default theme ID - resulting in storing '' in the localStorage and setting NO css class in DOM in case when this theme is activated
  5. Fixed the mechanism of StatePersistenceService (the function persistToStorage to be precise) to allow for storing an empty string '' - which, to my surprise, was previously impossible (due to a condition if(value)...). Now only the undefined values are prevented. For more, see the JSDoc comment with explanation in persistToStorage() function
  6. Removed the whole NGRX code for setting/getting possible Themes. Now all Themes can be determined synchronously based on configs.
  7. When a customer doesn't configure manually the default theme's ID (config.context.theme) and when they don't enable fetching a theme from CMS (via StatePersistenceService), then we we use a fallback value - an empty string '' - as the css class / ID of the default theme. This matches the original behavior on the develop branch - to not set any css class when config.context.theme is not defined.

QA steps:
Prerequisites:

  • you need to display a ThemeSwitcher component in every page (it's a CMS component but currently we're lacking sample data for it in our dev server) - here is a quick way to do it:
    • in storefront-component.module.ts line 29, add an import of the module ThemeSwitcherModule (from '../../cms-components/misc/theme-switcher/theme-switcher.module';)
    • in storefront.component.html line 1, add <cx-theme-switcher />
  • Open Spartacus and open DevTools -> Application tab -> local storage

TEST CASES:

  1. TEST CASE: When a default theme ID is NOT configured - config.context.theme is undefined
    1. (note: in our storefront app, no default theme ID is configured)
    2. Clear the entry for the key spartacus⚿⚿siteTheme, if exists.
    3. Refresh the page
    4. 👉 Verify that the entry spartacus⚿⚿siteTheme has a value '' (empty string)
    5. 👉 Verify that when you run in the console [...document.querySelector('cx-storefront').classList], the list doesn't contain any class related to theming
    6. In the top of the page in the ThemeSwitcher, change the theme to HC-dark
    7. 👉 Verify that the entry spartacus⚿⚿siteTheme has a value 'cx-theme-high-contrast-dark'
    8. 👉 Verify that when you run in the console [...document.querySelector('cx-storefront').classList], the list contains 'cx-theme-high-contrast-dark'
    9. Refresh the page
    10. 👉 Verify that the entry spartacus⚿⚿siteTheme has a value 'cx-theme-high-contrast-dark'
    11. 👉 Verify that when you run in the console [...document.querySelector('cx-storefront').classList], the list contains 'cx-theme-high-contrast-dark'
    12. In the top of the page in the ThemeSwitcher, change the theme back to Default
    13. 👉 Verify that the entry spartacus⚿⚿siteTheme has a value '' (empty string)
    14. 👉 Verify that when you run in the console [...document.querySelector('cx-storefront').classList], the list doesn't contain any class related to theming (e.g. it DOES NOT contain anymore 'cx-theme-high-contrast-dark')
  2. TEST CASE: When a default theme ID IS configured - config.context.theme is defined
    1. in spartacus-b2c-configuration.module.ts in line 40, in the context config, add a new key: theme: ['spike']
    2. 👉👉👉 verify all analogically as in the previous TEST CASE, but instead of '' you should see spike
  3. TEST CASE: When a default theme ID is automatically configured, based on CMS Basesites - config.context.theme is populated by the SiteContextConfigInitializer
    1. in spartacus-b2c-configuration.module.ts comment-out lines 36-40, effectively removing the static context config. (This will trigger loading loading this config from CMS Basesites. For more see docs)
    2. 👉👉👉 verify all analogically as in the previous TEST CASE, but instead of '' you should see santorini

closes https://jira.tools.sap/browse/CXSPA-8153

- Use `config.context[THEME_CONTEXT_ID]` as a default theme ID
- No longer fetch a "custom theme" from the backend, but use `config.context[THEME_CONTEXT_ID]` instead (which could have been loaded from backend by the `SiteContextConfigService`, but didn't have to)
- Use other configured themes (`config.siteTheme.siteThemes`) as as optional themes
- removed from default config of optional themes the default theme object placeholder
- thanks to above, the validation of theme (on setActive) is now synchronous
…t's now derived synchronously from configs `config.context[SITE_THEME_ID]` and `config.siteTheme.siteThemes`
…se now by convention the first item in the array is the default one
…s class one from DOM, but just don't apply new (empty) class
Comment on lines +65 to +72
if (this.featureConfigService.isEnabled('useNewSiteThemeSwitcher')) {
if (this.existingTheme) {
this.renderer.removeClass(element, this.existingTheme);
}
if (theme) {
this.renderer.addClass(element, theme);
this.existingTheme = theme;
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

New code accounts for the fact that either existingTheme or the new theme can be an empty string ''. But the native DOM API throws an error when we try to add a CSS class ''.

Moreover, if the old class was non-empty, but the new one is '', then we want to just unset the previous one.

Copy link
Contributor

@pawelfras pawelfras left a comment

Choose a reason for hiding this comment

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

LGTM. As we discussed offline, unit test adjustments are going to be provided separately

@kimhw0630
Copy link
Contributor

@Platonn
I just checked out this branch and tested it before reviewing the code changes.
The default theme doesn't get set

  1. Go to home page and select Dark/light theme.
  2. Open devTools from Chrome -> Application -> search for spartacus⚿⚿siteTheme
    Check the value for spartacus⚿⚿siteTheme (cx-theme-high-contrast-dark or cx-theme-high-contrast-light)
  3. Remove this key
  4. Refresh the browser
  5. Go to devTools again to check
    spartacus⚿⚿siteTheme value should be “santorini” but spartacus⚿⚿siteTheme isn’t being created.

Also, we can no longer switch back to the default theme after selecting a different one

@pawelfras
Copy link
Contributor

pawelfras commented Aug 29, 2024

@Platonn I just checked out this branch and tested it before reviewing the code changes. The default theme doesn't get set

  1. Go to home page and select Dark/light theme.
  2. Open devTools from Chrome -> Application -> search for spartacus⚿⚿siteTheme
    Check the value for spartacus⚿⚿siteTheme (cx-theme-high-contrast-dark or cx-theme-high-contrast-light)
  3. Remove this key
  4. Refresh the browser
  5. Go to devTools again to check
    spartacus⚿⚿siteTheme value should be “santorini” but spartacus⚿⚿siteTheme isn’t being created.

Also, we can no longer switch back to the default theme after selecting a different one

Good catch. It seems to me that both problems are caused by the lack of a default value of THEME_CONTEXT_ID. We could consider adding santorini as a default value to the default-site-context-config.ts.
EDIT: Only now I recalled that we don't need to set santorini CSS class to switch back to the default theme as it is default styling for Spartacus which doesn't require setting additional class:
image
Removing classes related to high-contrast themes should be enough.

@kimhw0630 kimhw0630 marked this pull request as ready for review September 2, 2024 15:19
@kimhw0630 kimhw0630 requested a review from a team as a code owner September 2, 2024 15:19
@pawelfras pawelfras merged commit 2077dd4 into feat/CXSPA-7604 Sep 6, 2024
4 checks passed
@pawelfras pawelfras deleted the feat/CXSPA-7604--fix-race-condition branch September 6, 2024 10:20
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.

4 participants