WARNING: THIS SITE IS A MIRROR OF GITHUB.COM / IT CANNOT LOGIN OR REGISTER ACCOUNTS / THE CONTENTS ARE PROVIDED AS-IS / THIS SITE ASSUMES NO RESPONSIBILITY FOR ANY DISPLAYED CONTENT OR LINKS / IF YOU FOUND SOMETHING MAY NOT GOOD FOR EVERYONE, CONTACT ADMIN AT ilovescratch@foxmail.com
Skip to content

Conversation

@sfc-gh-dmatthews
Copy link
Contributor

📚 Context

This PR passed the currently selected theme to all Community Cloud embedded apps as a query parameter to make them match the site theme.

Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.

@sfc-gh-dmatthews sfc-gh-dmatthews requested review from a team and removed request for sfc-gh-kmcgrady December 8, 2025 15:17
Copy link
Contributor

@sfc-gh-tteixeira sfc-gh-tteixeira left a comment

Choose a reason for hiding this comment

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

Approved after comments addressed

return "light"; // Default fallback for SSR
};

const currentTheme = getCurrentTheme();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the function needed?

return "light"; // Default fallback for SSR
};

const currentTheme = getCurrentTheme();
Copy link
Contributor

Choose a reason for hiding this comment

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

If currentTheme isn't used anywhere else, I think it's cleaner to just instantiate it to "dark_theme" or "light_theme" directly, rather than do a more brittle string interpolation later on.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually, just instantiate to embed_options=dark_theme directly, and avoid a second string interpolation below.

Same for light.

}

// Add theme parameter to embed query string
embedQueryStr += `&${themeParam}`;
Copy link
Contributor

Choose a reason for hiding this comment

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

It's cleaner to add the theme directly in the embedQueryParams array, and let the string joining logic above handle it.

That is, remove the if (query) { block but keep the code inside, modified like this:

    const embedQueryParams = [themeParam];
    const normalQueryParams = [];

    if (query) {
      query.split("&").forEach((qStr) => {
        if (qStr.startsWith("embed=") || qStr.startsWith("embed_options=")) {
          embedQueryParams.push(qStr);
        } else {
          normalQueryParams.push(qStr);
        }
      });
    }

    embedQueryStr = "&" + embedQueryParams.join("&");
    normalQueryStr = "&" + normalQueryParams.join("&");

@sfc-gh-dmatthews
Copy link
Contributor Author

sfc-gh-dmatthews commented Dec 9, 2025

I think I followed your suggestions. I couldn't get rid of the function entirely, because server-side rendering broke. So, re-requested a review to make sure it satisfies you requests.

@sfc-gh-dmatthews
Copy link
Contributor Author

Actually, the breaks a portion. Apps from docstrings only load in light mode. I'll update it again.

@sfc-gh-dmatthews sfc-gh-dmatthews force-pushed the fix/set-apps-from-site-theme branch from 95fdbe4 to a2a8a40 Compare December 14, 2025 01:53
@sfc-gh-dmatthews
Copy link
Contributor Author

I changed it to use a theme context to so that API pages would load with the correct theme. The first fix had the following problem:

  1. Load in light theme
  2. Switch to dark theme
  3. Navigate to API page
  4. Apps were light, but could toggle to light and back to dark to make them dark

The new fix will load the right them for the API apps, even if the theme was changed sometime after the first load.

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.

3 participants