Repository navigation
Conversation
|
Thanks for the pull request, @rpenido! This repository is currently maintained by 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 approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo 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:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. 🔘 Update the status of your PRYour PR is currently marked as a draft. After completing the steps above, update its status by clicking "Ready for Review", or removing "WIP" from the title, as appropriate. 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:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3271 +/- ##
========================================
Coverage 96.12% 96.12%
========================================
Files 1428 1428
Lines 34717 34771 +54
Branches 8295 8037 -258
========================================
+ Hits 33372 33425 +53
- Misses 1304 1305 +1
Partials 41 41 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
3a3f6d1 to
d6cddae
Compare
d6cddae to
a48590d
Compare
a48590d to
e235716
Compare
37a21e6 to
fc5aa20
Compare
fc5aa20 to
30179e5
Compare
30179e5 to
b925d3d
Compare
| const urls = [pickUrl(themeUrls.core), pickUrl(variant)] | ||
| .filter((url): url is string => Boolean(url)); |
There was a problem hiding this comment.
@rpenido This method only includes the two custom override css files. However, I believe we also need the base paragon css urls included too. For normal operation (I know theming is in active development, so it could change in future), this should end up as an array of 4 urls:
- https://cdn.jsdelivr.net/npm/@openedx/paragon@23/dist/core.min.css
- https://custom.brandOverride.example.com/core.css
- https://cdn.jsdelivr.net/npm/@openedx/paragon@23/dist/light.min.css
- https://custom.brandOverride.example.com/light.css
cc @xitij2000
| if (!urls.length) { | ||
| return tinyMCEStyles; | ||
| } | ||
| return `${urls.map((url) => `@import url("${url}");`).join('\n')}\n${tinyMCEStyles}`; |
There was a problem hiding this comment.
@rpenido the trouble with loading the tinymce content styles after the theming, is that the tinymce content styles will override a lot of theme styling.
It's tricky though, because the tinymce content styles include accessibility and functionality for editing, as well as setting unwanted visual styles here. I wonder if tinymce has options to turn off the visual styles so we can use the mfe theme here? Or maybe we can manually include just the functional/a11y css from tinymce content styles?
| const assetFormatRegex = /\/asset-v1:\S+[+]\S+[@]\S+[+]\S+[@]/; | ||
| const isCorrectAssetFormat = assetFormatRegex.test(assetSrc); |
There was a problem hiding this comment.
Flagging an unrelated refactoring
| : 'olx' in content | ||
| ? { ...content, olx: content.olx.replace(imageBS64, imagePath) } | ||
| : { ...content, data: content.data.replace(imageBS64, imagePath) }; |
| <Form.Switch | ||
| name="include_theme" | ||
| checked={includeTheme} | ||
| onChange={handleIncludeThemeChange} | ||
| floatLabelLeft | ||
| className="mb-0" | ||
| > |
There was a problem hiding this comment.
@rpenido I think this could benefit from some short explanation text about the implications of this (especially since the differences are greater than just including the theme - there is shadowroot sandboxing as well)? It may be worth pinging Cassie or Ali for UI/UX review about the toggle too.
Description
Adds a Use MFE Theme toggle to the Text (HTML) block editor in Studio, so an author can opt an individual text block into the deployment's theme. The block's learner view (openedx/xblocks-core#308) renders opted-in content inside a shadow root with the Paragon theme attached, so page styles cannot reach the content and the content cannot leak styles back out.
User roles impacted: Course Author (new opt-in toggle on the Text block; the editing area picks up the theme when it is on).
Supporting information
include_themefield and the learner-view rendering.include_themeon export, so the setting silently reverts after a course export/import.Testing instructions
Other information
Best Practices Checklist
.ts,.tsx).propTypesanddefaultPropsin any new or modified code.src/testUtils.tsx(specificallyinitializeMocks)apiHooks.tsin this repo for examples.messages.tsfiles have adescriptionfor translators to use.../in import paths. To import from parent folders, use@src, e.g.import { initializeMocks } from '@src/testUtils';instead offrom '../../../../testUtils'Private ref: FAL-4394