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.20% 96.20%
=======================================
Files 1446 1446
Lines 35384 35442 +58
Branches 8179 8200 +21
=======================================
+ Hits 34041 34098 +57
- Misses 1301 1302 +1
Partials 42 42 ☔ 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?
There was a problem hiding this comment.
It's tricky though, because the tinymce content styles include accessibility and functionality for editing, as well as setting unwanted visual styles here.
I'm not sure if I got what you mean here. Do you mean the css from src/editors/data/constants/tinyMCEStyles.js that is in the tinyMCEStyles variable or something else (like a native CSS from the component)?
| const assetFormatRegex = /\/asset-v1:\S+[+]\S+[@]\S+[+]\S+[@]/; | ||
| const isCorrectAssetFormat = assetFormatRegex.test(assetSrc); |
There was a problem hiding this comment.
Flagging an unrelated refactoring
There was a problem hiding this comment.
Just to add context: I changed this as part of the js -> tsx conversion. isCorrectAssetFormat was boolean | undefined before.
Also, calling match would extract all groups, which we don't need here. Using test in this context is cheaper and leaner.
| : 'olx' in content | ||
| ? { ...content, olx: content.olx.replace(imageBS64, imagePath) } | ||
| : { ...content, data: content.data.replace(imageBS64, imagePath) }; |
There was a problem hiding this comment.
This code was used to replace base64 added images with assets. Since I changed the structure for the text blocks (data is not a string anymore), this would crash while accessing content.olx.replace. I just added a guard for the access.
Furthermore, this code is called in the context of batchUploadAssets, which requires creating a new xblock with content. I think we always create an empty block now, so this whole thing is probably a dead feature.
| <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.
c1c8a10 to
371a73d
Compare
Description
Adds a Use MFE theming toggle to the Text (HTML) block editor in Studio, so an author can control the deployment's theme per block. New blocks start with theming on, so the feature is opt-out; an existing block that has never stored the setting starts with it off. 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 toggle on the Text block, on by default for new blocks; the editing area picks up the theme when it is on).
Supporting information
use_mfe_themingfield and the learner-view rendering.use_mfe_themingon 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