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.

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!

What's next?

Please work through the following steps to get your changes 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.

🔘 Let us know that your PR is ready for review:

Who will review my changes?

This repository is currently maintained by @openedx/committers-frontend. Tag them in a comment and let them know that your changes are ready for review.

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 83.90093% with 52 lines in your changes missing coverage. Please review.

Project coverage is 83.64%. Comparing base (bc9c233) to head (837858e).

Files	Patch %	Lines
src/react/hooks/paragon/useParagonThemeVariants.js	81.57%	19 Missing and 2 partials ⚠️
src/react/hooks/paragon/useParagonThemeCore.js	80.95%	15 Missing and 1 partial ⚠️
src/react/hooks/paragon/utils.js	78.78%	7 Missing ⚠️
src/react/hooks/paragon/useParagonTheme.js	84.37%	4 Missing and 1 partial ⚠️
src/react/reducers.js	84.61%	2 Missing ⚠️
src/react/hooks/paragon/useParagonThemeUrls.js	96.77%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #689      +/-   ##
==========================================
+ Coverage   83.41%   83.64%   +0.23%     
==========================================
  Files          40       48       +8     
  Lines        1073     1388     +315     
  Branches      197      290      +93     
==========================================
+ Hits          895     1161     +266     
- Misses        166      211      +45     
- Partials       12       16       +4

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

GlugovGrGlib · 2024-05-11T06:59:39Z

docs/how_tos/theming.md

@@ -0,0 +1,204 @@
+# Theming support with `@edx/paragon` and `@edx/brand`


we need to refactor @edx/ to @openedx/ in docs, as since December npm packages for @openedx/paragon, @openedx/frontend-build, @openedx/frontend-plugin-framework, @openedx/brand-openedx
are published in the scope of @openedx/

@brian-smith-tcril I couldn't find frontend-platform among npm published packages, shouldn't it be published with @openedx scope as well?
https://www.npmjs.com/search?q=%40openedx

I think that one is still only published to the @edx scope. I agree it would be good to make the switch but it hasn't made it to the top of the priority list yet.

GlugovGrGlib · 2024-05-11T07:08:34Z

package.json

-    "@openedx/frontend-build": "14.0.3",
-    "@openedx/paragon": "22.3.2",
+    "@openedx/frontend-build": "github:edunext/frontend-build#dcoa/design-tokens-support",
+    "@openedx/paragon": "npm:@openedx/paragon@^22.0.0-alpha.25",


It might worth to wait for the Paragon sync PR to be merged, so we won't downgrade paragon requirements, and still will be able to use the latest version of paragon.

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.

PKulkoRaccoonGang · 2024-05-24T13:28:17Z

docs/how_tos/theming.md

+`@openedx/frontend-platform` supports both `light` (required) and `dark` (optional) theme variants. The choice of which theme variant should be applied on page load is based on the following preference cascade:
+
+1. **Get theme preference from localStorage.** Supports persisting and loading the user's preference for their selected theme variant, until cleared.
+1. **Detect user system settings.** Rely on the `prefers-color-scheme` media query to detect if the user's system indicates a preference for dark mode. If so, use the default dark theme variant, if one is configured.


[question]: Is there now support for adding a dark/light theme (or other custom themes) when using locally installed Paragon CSS?

For example:

@use "@openedx/paragon/dist/core.min.css" as paragonCore; @use "@openedx/paragon/dist/light.min.css" as paragonLight; @use "@openedx/paragon/dist/dark.min.css" as paragonDark;

This works great using JS and MFE runtime configurations, but I have not been able to test it for local themes.

The hook will fallback in the installed themes files if the URL is not provided or is not working.

If you define only the CSS inside SCSS file you can use the prefers-color-scheme to support dark/light modes. But you can not create a custom switch.

Thank you! It would be great to explicitly indicate exactly how it is possible to use different imports for a light/dark theme in the documentation. I plan to do this in the migration documentation and for Paragon.

For example:

@use "@openedx/paragon/dist/core.min.css" as paragonCore; @import url('@openedx/paragon/dist/light.min.css') (prefers-color-scheme: light); @import url('@my-brand/dark.min.css') (prefers-color-scheme: dark);

100% agree that the information in your example should be part of the Paragon and migration documents.

This is a different approach, the explanation of how to use variants is below, in the title Basic theme URL configuration.

I will improve one of the examples to use dark mode, to make it more explicit.

PKulkoRaccoonGang · 2024-05-24T13:44:54Z

docs/how_tos/theming.md

+
+### Falling back to styles installed in consuming application
+
+If any of the configured external `PARAGON_THEME_URLS` fail to load for whatever reason (e.g., CDN is down, URL is incorrectly configured), `@openedx/paragon` will attempt to fallback to the relevant files installed in `node_modules` from the consuming application.


When trying to make sure that if all themes are not loaded successfully, themes from the local version of Paragon will be loaded, I encountered the following problems:

@PKulkoRaccoonGang thanks for the feedback, the fallback url was missing the publicPath I solved it.

We don’t receive undefined, but I couldn’t load styles from the locally installed Paragon package in node_modules. Could you check if getParagonThemeCss works correctly in a PR related to a frontend-build?

The problem here is related to tutor-mfe, they define BASE_URL as the MFE_HOST (you can see here) thing that is not 100% true in dev mode because the URL needs the PORT that is specific for each MFE (you can see here )

You can add in the settings:

MFE_CONFIG_OVERRIDES = { "discussions": { "BASE_URL": "http://apps.local.edly.io:2002" } }

PKulkoRaccoonGang · 2024-05-24T13:49:56Z

docs/how_tos/theming.md

+### Basic theme URL configuration
+
+Paragon supports 2 mechanisms for configuring the Paragon theme urls:
+* JavaScript-based configuration via `env.config.js`


[question]: This approach works great in MFE, but I encountered a problem in frontend-build/example, unfortunately, the styles are not loading for me. Is everything working correctly?

For example: I tried adding a Button component with Paragon and noticed that the styles with Paragon were not being applied to this component

PKulkoRaccoonGang · 2024-05-24T13:53:22Z

docs/how_tos/theming.md

+
+```shell
+https://cdn.jsdelivr.net/npm/@openedx/paragon@$paragonVersion/dist/core.min.css
+https://cdn.jsdelivr.net/npm/@openedx/paragon@$paragonVersion/dist/light.min.css


[question]: Is it worth adding an example of importing in SCSS format here? This also applies to the local import example @openedx/brand-openedx

For example:

@use "@openedx/paragon/dist/core.min.css" as paragonCore; @use "@openedx/paragon/dist/light.min.css" as paragonLight;

No, because the main intention here is to identify the version and load the external resource compatible with that specific version.

For example, my app has paragon 23.0.0-alpha.2, then the hook will replace $paragonVersion .

This is the definition
https://cdn.jsdelivr.net/npm/@openedx/paragon@$paragonVersion/dist/core.min.css

after processing
https://cdn.jsdelivr.net/npm/@openedx/paragon@$23.0.0-alpha.2/dist/core.min.css

Thanks for the explanation 💯

PKulkoRaccoonGang · 2024-05-24T14:06:19Z

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

+    global.PARAGON_THEME = {
+      paragon: {
+        version: '1.0.0',
+        themeUrls: {
+          core: {
+            fileName: 'core.min.css',
+          },
+          defaults: {
+            light: 'light',
+          },
+          variants: {
+            light: {
+              fileName: 'light.min.css',
+            },
+          },
+        },
+      },
+    };


[nit]: We also use this configuration for the test in the file src/react/hooks/paragon/useParagonThemeVariants.test.js
I suggest putting this code into a separate file and reusing it to test both hooks

I can get rid of this understanding that is being exported by frontend-build (including for testing)

Note: Once the frontend-build PR has been merged we need to update the devDependencies and peerDependencies

PKulkoRaccoonGang · 2024-05-24T14:19:39Z

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

+    });
+
+    it('returns expected object when configuration is valid (both Paragon + brand)', () => {
+      const config = {


[optional]: I suggest reusing the config constant, which refers to an object with the necessary configuration for tests in this file. This will allow us to get rid of code duplication. What do you think?

I prefer leaving this section as it is. The important part here is PARAGON_THEME_URLS, which changes in each test. It's easier to understand this way, I think.

Good, I agree with you, thanks

src/react/hooks/paragon/useParagonThemeUrls.js

src/react/AppProvider.test.jsx

PKulkoRaccoonGang · 2024-05-24T14:32:38Z

src/react/AppProvider.test.jsx

+    });
+
+    it('calls useParagonTheme', () => {
+      const Component = (


[question]: Can we declare a global Сomponent constant for a block with paragon theme and brand tests and reuse it in tests?

I can declare the global component but this test doesn't cover the useParagonTheme behavior so it is not necessary to declare paragon theme and brand. This is focused on the setThemeVariant function and the theme state.

src/react/hooks/paragon/useParagonThemeUrls.js

feat: adds support for loading external theme CSS for MFEs Introduces `useAppTheme` in `AppProvider` to load/inject `<link>` elements for the core theme CSS and any theme variant CSS into the HTML document. Exposes the app theme state and a way to mutate it to consumers via `AppContext`. fix: handle missing theme config fix: add env vars to env files fix: remove unused code chore: wip commit fix: prefer runtime config for paragon theme fix: rebase on master and resolve package conflicts fix: revert to original webpack.dev.config config chore: clean up unnecessary comment line fix: grammar Co-authored-by: Adolfo R. Brandes <[email protected]>

chore: refresh package-lock.json to lockfileVersion 3 fix: clean up link nodes in document head when no longer needed fix: fallback to locally installed core and light theme css urls docs: update to docs chore: update docs about fix: expose setThemeVariant

fix: rebase with master and update based on PARAGON changes chore: remove support for env vars config for paragon dynamic theming

fix: updates fix: one more update fix: refresh package-lock.json fix: refresh package-lock.json pt2 fix: updates fix: update package-lock.json fix: make it theme variant agnostic docs: update howto theming guide fix: ensure app loads without styles if the PARAGON_THEME_URLS and fallback urls all fail to load fix: ensure fallback theme links are removed if they also error docs: add link to mfe runtime config api adr fix: don't attempt to load paragon css urls if PARAGON_THEME_URLS is absent

docs: fix code example docs: add missing comma docs: update how to

chore: update package-lock.json chore: update package-lock.json take 2 chore: remove console.log statements fix: ignore system preference change when theme variant set in localstorage chore: add tests for updates to AppProvider chore: update react-intl to pass peer dependencies after pinning all deps chore: split hooks.js up into separate files and begin some related tests test: add testing to useParagonTheme hooks (openedx#514) * test: add testing to useParagonThemeCore * test: add test to useThemeVariants hook * fix: Paragon definition and remove onload mock * test: change test message to be clear

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

brian-smith-tcril · 2024-07-30T15:58:12Z

package.json

@@ -35,11 +35,12 @@
  "devDependencies": {
    "@edx/brand": "npm:@openedx/brand-openedx@^1.2.2",
    "@edx/browserslist-config": "1.2.0",
-    "@openedx/frontend-build": "14.0.10",
+    "@openedx/frontend-build": "github:edunext/frontend-build#dcoa/design-tokens-support",


Note
Waiting on

feat: add ParagonWebpackPlugin to support design tokens frontend-build#546

brian-smith-tcril · 2024-07-30T16:05:17Z

package.json

@@ -76,7 +77,7 @@
  },
  "peerDependencies": {
    "@openedx/frontend-build": ">= 14.0.0",
-    "@openedx/paragon": ">= 21.5.7 < 23.0.0",
+    "@openedx/paragon": " 23.0.0-alpha.1 || >= 21.5.7 < 23.0.0",


Note
Waiting on Paragon alpha to be merged to master

…nts hook

arbrandes · 2024-09-27T15:17:40Z

@davidjoy, this is a doozy to keep an eye on.

davidjoy · 2024-10-16T15:04:00Z

Sure is. It looks like it was temporary, but just to explicitly note it: the dist folder has been checked in in this PR. We need to remember to remove that before merging.

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

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

adamstankiewicz and others added 11 commits June 26, 2024 11:28

fix: remove duplicate import

4727a2f

chore: refresh package-lock.json to lockfileVersion 3 fix: clean up link nodes in document head when no longer needed fix: fallback to locally installed core and light theme css urls docs: update to docs chore: update docs about fix: expose setThemeVariant

docs: update docs

0eb957e

fix: rebase with master and update based on PARAGON changes chore: remove support for env vars config for paragon dynamic theming

fix: brand overrides

17c0b8c

docs: fix code example docs: add missing comma docs: update how to

fix: update dependenices and AppProvider test

b64c154

style: fix variable name and funtion

d934ec7

test: add useParagonTheme test

d170103

docs: update scope of paragon and brand for openedx

89104fd

refactor: allow multiple replacement in version replacement

0445ee2

dcoa added 4 commits June 26, 2024 11:35

refactor: update webpack dev config

33aba81

refactor: avoid duplicated links if the theme varian is preloaded

5dd20a1

refactor: update useParagonTheme when some variables are missing

ba3f639

test: add new test csse for useParagonThemeCore

4c0ea76

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

Draft

4 tasks

brian-smith-tcril reviewed Jul 30, 2024

View reviewed changes

dcoa added 8 commits August 12, 2024 20:07

refactor: use modules for config file

7ea2c6a

fix: fallback url

65144e0

style: fix imports order

a269c5d

refactor: create isEmptyObject and fallbackThemeUrl utils

4e66dc4

test: refactor testing fot app provider, theme core hook, theme varia…

b2bfe7c

…nts hook

docs: improve themening documentation

2e1da80

chore: update dependencies and regenerate package-lock

8f84e1c

temp: add dist folder for testing purposes

ae5b53f

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

refactor: allow env PARAGON_THEME_URLS definition

3576ee7

dcoa force-pushed the dcoa/design-tokens-support branch from 0b4c547 to 3576ee7 Compare August 14, 2024 23:45

awais-ansari requested review from GlugovGrGlib, PKulkoRaccoonGang and brian-smith-tcril October 9, 2024 11:44

		@@ -0,0 +1,204 @@
		# Theming support with `@edx/paragon` and `@edx/brand`


		### Falling back to styles installed in consuming application

		If any of the configured external `PARAGON_THEME_URLS` fail to load for whatever reason (e.g., CDN is down, URL is incorrectly configured), `@openedx/paragon` will attempt to fallback to the relevant files installed in `node_modules` from the consuming application.

		@@ -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

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

What's next?

🔘 Get product approval

🔘 Provide context

🔘 Get a green build

🔘 Let us know that your PR is ready for review:

Who will review my changes?

Where can I find more information?

When can I expect my changes to be merged?

codecov bot commented May 10, 2024 • edited Loading

Codecov Report

GlugovGrGlib May 11, 2024 • edited Loading

Choose a reason for hiding this comment

GlugovGrGlib May 11, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PKulkoRaccoonGang commented May 20, 2024

dcoa commented May 21, 2024 • edited Loading

PKulkoRaccoonGang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcoa May 27, 2024 • 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

Choose a reason for hiding this comment

dcoa Jun 1, 2024 • 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

Choose a reason for hiding this comment

dcoa May 28, 2024 • 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

Choose a reason for hiding this 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

arbrandes commented Sep 27, 2024

davidjoy commented Oct 16, 2024

dcoa commented May 10, 2024 •

edited

Loading

openedx-webhooks commented May 10, 2024 •

edited

Loading

codecov bot commented May 10, 2024 •

edited

Loading

GlugovGrGlib May 11, 2024 •

edited

Loading

GlugovGrGlib May 11, 2024 •

edited

Loading

dcoa commented May 21, 2024 •

edited

Loading

dcoa May 27, 2024 •

edited

Loading

dcoa Jun 1, 2024 •

edited

Loading

dcoa May 28, 2024 •

edited

Loading