feat: adds support for loading external theme CSS for MFEs #689

dcoa · 2024-05-10T06:26:11Z

Description:

This PR updates the original one #440 closer to the master branch and adds some extra tests.

Please read the original PR for additional context.

Warning

The dist folder is included in the PR for testing purposes and will be removed before the merge.
The PR should update the Paragon version once the official release is launched.

Merge checklist:

Consider running your code modifications in the included example app within frontend-platform. This can be done by running npm start and opening http://localhost:8080.
Consider testing your code modifications in another local micro-frontend using local aliases configured via the module.config.js file in frontend-build.
Verify your commit title/body conforms to the conventional commits format (e.g., fix, feat) and is appropriate for your code change. Consider whether your code is a breaking change, and modify your commit accordingly.

Post merge:

After the build finishes for the merged commit, verify the new release has been pushed to NPM.

openedx-webhooks · 2024-05-10T06:26:16Z

Thanks for the pull request, @dcoa!

This repository is currently maintained by @openedx/committers-frontend.

Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.

🔘 Get product approval

If you haven't already, check this list to see if your contribution needs to go through the product review process.

If it does, you'll need to submit a product proposal for your contribution, and have it reviewed by the Product Working Group.
- This process (including the steps you'll need to take) is documented here.
If it doesn't, simply proceed with the next step.

🔘 Provide context

To help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:

Dependencies

This PR must be merged before / after / at the same time as ...
Blockers

This PR is waiting for OEP-1234 to be accepted.
Timeline information

This PR must be merged by XX date because ...
Partner information

This is for a course on edx.org.
Supporting documentation
Relevant Open edX discussion forum threads

🔘 Get a green build

If one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green.

Where can I find more information?

If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources:

When can I expect my changes to be merged?

Our goal is to get community contributions seen and reviewed as efficiently as possible.

However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:

The size and impact of the changes that it introduces
The need for product review
Maintenance status of the parent repository

💡 As a result it may take up to several weeks or months to complete a review and merge your PR.

codecov · 2024-05-10T06:28:42Z

Codecov Report

Attention: Patch coverage is 96.75516% with 11 lines in your changes missing coverage. Please review.

Project coverage is 86.71%. Comparing base (b0774f2) to head (5db22d0).
Report is 4 commits behind head on master.

Files with missing lines	Patch %	Lines
src/config.js	44.44%	4 Missing and 1 partial ⚠️
src/react/hooks/paragon/useParagonThemeCore.js	97.59%	2 Missing ⚠️
src/react/hooks/paragon/useParagonThemeVariants.js	98.23%	2 Missing ⚠️
src/react/reducers.js	84.61%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
+ Coverage   83.41%   86.71%   +3.30%     
==========================================
  Files          40       48       +8     
  Lines        1073     1393     +320     
  Branches      197      292      +95     
==========================================
+ Hits          895     1208     +313     
- Misses        166      172       +6     
- Partials       12       13       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

docs/how_tos/theming.md

package.json

PKulkoRaccoonGang · 2024-05-20T10:24:21Z

@dcoa I'm getting an error during build related to the env.config.js file

SyntaxError: Unexpected token 'export'

I tried to solve it by changing the export to module.exports = { config }; I succeeded, but the content of the example application is not rendered on http://localhost:8080.

Have you encountered such a problem?

dcoa · 2024-05-21T11:12:36Z

@PKulkoRaccoonGang this is related to frontend-build changes, I'm checking right now

This line is making that the app is not rendering in frontend-platform https://github.com/openedx/frontend-build/pull/546/files#diff-22e13ddf245ea4fa81ca4d686dddf46fa9cbf70fbbff99991cccbf0c8ff82316R175

If you remove it, you will be able to see the app.

Update

I made an update to webpack dev config for example app and is working now.

PKulkoRaccoonGang

@dcoa This looks great! I have already started working on Update openedx/frontend-app-discussions. I left a few questions about the operation of some features and the code in general.

docs/how_tos/theming.md

src/react/hooks/paragon/useParagonThemeCore.test.js

src/react/hooks/paragon/useParagonThemeUrls.test.js

src/react/hooks/paragon/useParagonThemeUrls.js

src/react/AppProvider.test.jsx

src/react/hooks/paragon/useParagonThemeUrls.js

brian-smith-tcril

Overall this is looking great! I left a suggestion to clarify the documentation is referring to design tokens, and a few comments noting PRs that need to land before this can.

Thank you so much for this!

brian-smith-tcril · 2024-07-30T15:54:21Z

docs/how_tos/theming.md

@@ -0,0 +1,250 @@
+# Theming support with `@openedx/paragon` and `@openedx/brand-openedx`


Suggested change

# Theming support with `@openedx/paragon` and `@openedx/brand-openedx`

# Theming support with `@openedx/paragon` and `@openedx/brand-openedx`

> [!IMPORTANT]

> This document describes theming with design tokens.

> Information on theming MFEs that do not yet have design tokens support:

> * https://github.com/openedx/brand-openedx

> Information on the design tokens project:

> * https://github.com/openedx/paragon/blob/master/docs/decisions/0019-scaling-styles-with-design-tokens.rst

> * https://github.com/openedx/paragon/tree/alpha?tab=readme-ov-file#design-tokens

It looks like the formatting on this is a little bit broken

I think adding some newlines should clean it up

package.json

src/react/hooks/paragon/useParagonThemeVariants.js

src/config.js

src/react/hooks/paragon/useParagonTheme.js

adamstankiewicz · 2025-02-03T17:13:18Z

src/react/AppProvider.jsx

+  if (!paragonThemeState?.isThemeLoaded) {
+    return null;
+  }


[sanity check] Given when this code path is executed, the entire MFE app is rendered as a blank white screen, I'm wondering if we have sufficient observability for when we return null here, after any actual async loading has finished/resolved.

For example, if there's an issue with the CDN and we also fail to fallback to using the CSS source files from the locally installed @openedx/paragon and @edx/brand packages, and the MFE returns a blank white screen, will we sufficiently get alerted to it via logError, etc.?

I think we have enough paths for logging potential errors during the loading, I will working on the testing that will help to cover that cases and potentially identify if some validation is missing

From discussion during Paragon WG:

logging in useEffect to avoid duplicate logs

differentiate between error state and loading state

@brian-smith-tcril to look into what logs show up in the console for existing error states to get a sense of a reasonable logging strategy for this

possible future improvement (once React 18 support lands) - use suspense?

src/react/hooks/paragon/useParagonThemeCore.js

adamstankiewicz · 2025-02-03T17:19:05Z

src/react/hooks/paragon/useParagonThemeCore.js

+          logError(`Failed to load core theme CSS from ${url}`);
+          if (isFallbackThemeUrl) {
+            logError(`Could not load core theme CSS from ${url} or fallback URL. Aborting.`);


[curious] Any concerns around having two logError function calls within the same code path? I.e., if isFallbackThemeUrl is truthy here, would we have logged basically the same error twice (with slightly differing error messages)?

Should we just left the isFallbackThemeUrl and relay on the fallback logInfo as the can not load the default URL?

https://github.com/openedx/frontend-platform/pull/689/files#diff-6e8530492255f6f52120a1dd9f69147538c94d6e37be13541f802a2189f80af8R83

We can have a first message (in the logInfo - we also can replace it with a logError) that is
Could not load core theme CSS from ${url}. Falling back to locally installed core theme CSS: ${coreThemeFallbackUrl}

and then with the isFallbackThemeUrl the second message
Could not load core theme fallback URL. Aborting.

What do you think?

adamstankiewicz · 2025-02-03T17:20:22Z

src/react/hooks/paragon/useParagonThemeCore.js

+            if (isBrandOverride) {
+              setIsBrandThemeCoreLoaded(true);
+            } else {
+              setIsParagonThemeCoreLoaded(true);


[curious] Do we want to ensure coverage on the critical code paths within useParagonThemeCore, e.g. around loading? There's a few CodeCov annotations about missing coverage.

Yes, working on the overall coverage :)

adamstankiewicz · 2025-02-03T17:22:16Z

src/react/hooks/paragon/useParagonThemeVariants.js

+          logError(`Failed to load theme variant (${themeVariant}) CSS from ${value.urls.default}`);
+          if (isFallbackThemeUrl) {
+            logError(`Could not load theme variant (${themeVariant}) CSS from fallback URL. Aborting.`);


[curious] Similar question as above, regarding potential for calling logError twice for largely the same error, when isFallbackThemeUrl is truthy. Should we only call logError once in this case?

adamstankiewicz · 2025-02-03T17:22:46Z

src/react/hooks/paragon/useParagonThemeVariants.js

+          logError(`Failed to load theme variant (${themeVariant}) CSS from ${value.urls.default}`);
+          if (isFallbackThemeUrl) {
+            logError(`Could not load theme variant (${themeVariant}) CSS from fallback URL. Aborting.`);
+            if (isBrandOverride) {


Similar question around test coverage on some of the uncovered code paths within this hook.

src/react/index.js

brian-smith-tcril · 2025-02-04T17:00:38Z

src/config.js

@@ -137,6 +137,16 @@ function extractRegex(envVar) {
  return undefined;
 }

+function parseParagonThemeUrls(paragonUrlsJson) {


I think it makes sense to check for a falsy value before trying to parse. Not having process.env.PARAGON_THEME_URLS set is not an error state, but should still return {}.

Then, assuming we make it past the falsy check, we can go into the try/catch. It would also be good to log errors that we catch in there.

I think specifically checking for err being a SyntaxError exception (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/JSON/parse#exceptions) would be good. So something like

try { return JSON.parse(paragonUrlsJson); } catch (err) { if (err instanceof SyntaxError) { // log something about not being able to parse the json and it possibly being invalid. // maybe also log something about what to expect themes to look like when {} is returned return {}; } else { // not sure what to do here, if we're getting a different error type maybe we should just throw it? } }

[curious] Currently {} will not configure any URL (similar behavior as undefined), do we expect (prefer) a different behavior (for example, try to load the default themes)?

Discussed in Paragon WG: Following the same pattern as the CDN URLs being down (loading locally installed styles) seems like the best path forward here.

PARAGON_THEME_URLS Behavior

Doesn't exist (MFE choosing to use local styles) Load local styles (from MFE's style entrypoint)

Set, parses properly Attempt to load from parsed URL(s), if error - fallback to local styles

Set, fails to parse Fallback to local styles (injected from frontend-platform - derived from node modules)

…r Paragon hooks

…rocessing is done

dist/README.md

package.json

src/config.js

docs/how_tos/theming.md

brian-smith-tcril · 2025-02-18T15:16:36Z

@PKulkoRaccoonGang it looks like you have some unresolved comments from May of last year. Would you mind hitting the "Resolve conversation" button on the ones that don't require any more discussion or changes?

PKulkoRaccoonGang · 2025-02-19T06:41:37Z

@brian-smith-tcril I would love to, but I don’t have such a button. I think it’s because I don’t have the necessary permissions in this repository.

PKulkoRaccoonGang · 2025-02-19T06:43:43Z

@dcoa You can close all the comments I wrote where I added "Resolved" :)

brian-smith-tcril · 2025-02-19T14:58:59Z

@PKulkoRaccoonGang sorry about that! Thanks for commenting on the ones that have been resolved! I went through and clicked the button on them.

Co-authored-by: Brian Smith <[email protected]>

openedx-webhooks added the open-source-contribution PR author is not from Axim or 2U label May 10, 2024

dcoa force-pushed the dcoa/design-tokens-support branch from aee582b to e81b549 Compare May 11, 2024 00:23

dcoa requested a review from brian-smith-tcril May 11, 2024 00:49

dcoa changed the title ~~[WIP] feat: adds support for loading external theme CSS for MFEs~~ feat: adds support for loading external theme CSS for MFEs May 11, 2024

dcoa requested a review from adamstankiewicz May 11, 2024 00:49

GlugovGrGlib reviewed May 11, 2024

View reviewed changes

docs/how_tos/theming.md Outdated Show resolved Hide resolved

GlugovGrGlib reviewed May 11, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

dcoa force-pushed the dcoa/design-tokens-support branch from 5b4c843 to ed293e3 Compare May 14, 2024 00:44

dcoa force-pushed the dcoa/design-tokens-support branch from 9ebae07 to 94e4a62 Compare May 23, 2024 23:06

dcoa mentioned this pull request May 24, 2024

Finalize work on PR 440 (https://github.com/openedx/frontend-platform/pull/440) openedx/paragon#3061

Open

PKulkoRaccoonGang reviewed May 24, 2024

View reviewed changes

dcoa force-pushed the dcoa/design-tokens-support branch 4 times, most recently from a6a9211 to 229d79a Compare June 3, 2024 03:24

dcoa force-pushed the dcoa/design-tokens-support branch 3 times, most recently from 06fc4e6 to 837858e Compare June 26, 2024 02:24

brian-smith-tcril mentioned this pull request Jul 29, 2024

chore: test build and platform updates openedx/frontend-app-discussions#731

Closed

4 tasks

brian-smith-tcril reviewed Jul 30, 2024

View reviewed changes

dcoa force-pushed the dcoa/design-tokens-support branch from 2565f02 to ae5b53f Compare August 12, 2024 10:09

bra-i-am mentioned this pull request Aug 12, 2024

refactor: update pkg with the latest updates of upstream eduNEXT/frontend-platform#8

Merged

1 task

dcoa force-pushed the dcoa/design-tokens-support branch from 4204364 to 45642c7 Compare February 3, 2025 10:37

brian-smith-tcril reviewed Feb 3, 2025

View reviewed changes

src/react/hooks/paragon/useParagonThemeVariants.js Outdated Show resolved Hide resolved

adamstankiewicz reviewed Feb 3, 2025

View reviewed changes

refactor: clean up unnecessary changes

54c6e00

dcoa force-pushed the dcoa/design-tokens-support branch from 45642c7 to 54c6e00 Compare February 4, 2025 04:07

refactor: create a function to parse the JSON env for paragon

bc22970

dcoa force-pushed the dcoa/design-tokens-support branch from b2d3856 to bc22970 Compare February 4, 2025 06:08

brian-smith-tcril reviewed Feb 4, 2025

View reviewed changes

refactor: use getConfig diractly in useParagonUrls

8300338

dcoa force-pushed the dcoa/design-tokens-support branch from a29f484 to 8300338 Compare February 5, 2025 01:17

dcoa added 2 commits February 6, 2025 15:35

refactor: validate paragonUrlsJson and remove direct module export fo…

e3211d9

…r Paragon hooks

test: increase coverage useParagonTheme

1cff3e4

dcoa force-pushed the dcoa/design-tokens-support branch from 2179fbf to 1cff3e4 Compare February 6, 2025 05:34

dcoa added 2 commits February 7, 2025 16:48

refactor: change "loaded" for "complete" to reference when the link p…

040ca83

…rocessing is done

test: increase coverage for theme variants

a62e8db

PKulkoRaccoonGang requested a review from ihor-romaniuk February 9, 2025 11:07

PKulkoRaccoonGang reviewed Feb 9, 2025

View reviewed changes

dist/README.md Show resolved Hide resolved

package.json Outdated Show resolved Hide resolved

src/config.js Show resolved Hide resolved

dcoa added 4 commits February 11, 2025 20:37

test: increase test coverage

46448d2

chore: allow minor/patch paragon updates

d628f05

refactor: log error message when the JSON has a syntax error

85fb31e

docs: enhance JSDocs for the paragon functions

176c899

This was referenced Feb 12, 2025

[WIP] chore: update paragon to a version to 23 openedx/frontend-lib-special-exams#159

Draft

[BD-46] feat: added support Paragon design tokens openedx/frontend-component-header#351

Draft

brian-smith-tcril reviewed Feb 18, 2025

View reviewed changes

docs/how_tos/theming.md Outdated Show resolved Hide resolved

dcoa and others added 2 commits February 20, 2025 16:07

docs: improve readability of important highlights in theming.md

bd3695d

Co-authored-by: Brian Smith <[email protected]>

fix: replace onLoad with onComplete

5db22d0

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: adds support for loading external theme CSS for MFEs #689

feat: adds support for loading external theme CSS for MFEs #689

dcoa commented May 10, 2024 •

edited

Loading

openedx-webhooks commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

PKulkoRaccoonGang commented May 20, 2024

dcoa commented May 21, 2024 •

edited

Loading

PKulkoRaccoonGang left a comment

brian-smith-tcril left a comment

brian-smith-tcril Jul 30, 2024

brian-smith-tcril Feb 18, 2025

adamstankiewicz Feb 3, 2025

dcoa Feb 7, 2025 •

edited

Loading

brian-smith-tcril Feb 19, 2025

adamstankiewicz Feb 3, 2025

dcoa Feb 7, 2025 •

edited

Loading

adamstankiewicz Feb 3, 2025

dcoa Feb 6, 2025

adamstankiewicz Feb 3, 2025

adamstankiewicz Feb 3, 2025

brian-smith-tcril Feb 4, 2025 •

edited

Loading

dcoa Feb 11, 2025

brian-smith-tcril Feb 19, 2025

brian-smith-tcril commented Feb 18, 2025

PKulkoRaccoonGang commented Feb 19, 2025

PKulkoRaccoonGang commented Feb 19, 2025

brian-smith-tcril commented Feb 19, 2025

		@@ -0,0 +1,250 @@
		# Theming support with `@openedx/paragon` and `@openedx/brand-openedx`

-# Theming support with `@openedx/paragon` and `@openedx/brand-openedx`
+# Theming support with `@openedx/paragon` and `@openedx/brand-openedx`
+> [!IMPORTANT]
+> This document describes theming with design tokens.
+> Information on theming MFEs that do not yet have design tokens support:
+> * https://github.com/openedx/brand-openedx
+> Information on the design tokens project:
+> * https://github.com/openedx/paragon/blob/master/docs/decisions/0019-scaling-styles-with-design-tokens.rst
+> * https://github.com/openedx/paragon/tree/alpha?tab=readme-ov-file#design-tokens

`PARAGON_THEME_URLS`	Behavior
Doesn't exist (MFE choosing to use local styles)	Load local styles (from MFE's style entrypoint)
Set, parses properly	Attempt to load from parsed URL(s), if error - fallback to local styles
Set, fails to parse	Fallback to local styles (injected from `frontend-platform` - derived from node modules)

feat: adds support for loading external theme CSS for MFEs #689

Are you sure you want to change the base?

feat: adds support for loading external theme CSS for MFEs #689

Conversation

dcoa commented May 10, 2024 • edited Loading

openedx-webhooks commented May 10, 2024 • edited Loading

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

PKulkoRaccoonGang commented May 20, 2024

dcoa commented May 21, 2024 • edited Loading

PKulkoRaccoonGang left a comment

Choose a reason for hiding this comment

brian-smith-tcril left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoa Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoa Feb 7, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-smith-tcril Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brian-smith-tcril commented Feb 18, 2025

PKulkoRaccoonGang commented Feb 19, 2025

PKulkoRaccoonGang commented Feb 19, 2025

brian-smith-tcril commented Feb 19, 2025

dcoa commented May 10, 2024 •

edited

Loading

openedx-webhooks commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

dcoa commented May 21, 2024 •

edited

Loading

dcoa Feb 7, 2025 •

edited

Loading

dcoa Feb 7, 2025 •

edited

Loading

brian-smith-tcril Feb 4, 2025 •

edited

Loading