Add html lang prop to CodeEditor Component inside the SourceCodeModal - #2043
Conversation
|
Thanks for the pull request, @tonybusa! 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. DetailsWhere 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. |
There was a problem hiding this comment.
Pull Request Overview
This PR fixes an issue with the CodeEditor hook lookup by explicitly setting the language property to "html" on the CodeEditor component. The changes include updating the CodeEditor prop in the SourceCodeModal component and its corresponding snapshot to reflect this fix.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| src/editors/sharedComponents/SourceCodeModal/index.jsx | Added lang="html" to ensure proper lookup in CODEMIRROR_LANGUAGES |
| src/editors/sharedComponents/SourceCodeModal/snapshots/index.test.jsx.snap | Updated snapshot to reflect the new lang prop |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2043 +/- ##
========================================
Coverage 93.98% 93.99%
========================================
Files 1155 1155
Lines 24146 24177 +31
Branches 5116 5241 +125
========================================
+ Hits 22693 22724 +31
+ Misses 1385 1376 -9
- Partials 68 77 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
13445fe to
67b85c9
Compare
67b85c9 to
8248abb
Compare
|
@mphilbrick211: can you help us find a reviewer? This PR solves a high priority issue for the Teak release :) |
|
@openedx/committers-frontend - would someone be able to take a look at this? |
|
For additional context: I was not able to reproduce the issue on |
|
Sandbox deployment failed 💥 |
|
@brian-smith-tcril I explained the difference in this change in the "Other Information" section of the PR description. Sorry if you've already read that and your asking for a more clear explanation. But basically the fix you reference is defaulting to xml() and not actually using the lookup to get html(). Also referenced here: openedx/wg-build-test-release#466 (comment) |
brian-smith-tcril
left a comment
There was a problem hiding this comment.
Alright, so I think I'm finally fully following this now.
Before #1869 we had:
export const createCodeMirrorDomNode = ({
ref,
initialText,
upstreamRef,
lang,
}) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
const languageExtension = CODEMIRROR_LANGUAGES[lang]();and this instance of the CodeEditor component didn't have a lang prop set, causing the error.
That was changed in #1869 to be
export const createCodeMirrorDomNode = ({
ref,
initialText,
upstreamRef,
lang,
}) => {
// eslint-disable-next-line react-hooks/rules-of-hooks
useEffect(() => {
const languageExtension = CODEMIRROR_LANGUAGES[lang] ? CODEMIRROR_LANGUAGES[lang]() : xml();which fixes the error by handling the lack of lang prop by defaulting to xml.
This PR sets the lang prop in this instance to be html.
My only remaining question is if xml makes sense as the default or if the default should be html - but that question feels out of scope for this PR (and landing this PR won't break anything if we change the default to html in the future).
|
Sandbox deployment failed 💥 |
|
Sandbox deployment successful 🚀 |
|
Thank you all! @tonybusa: can you help us backporting this into the |
Hey @mariajgrimaldi, @Faraz32123 - should I backport this change and #1869 in the same PR? If the answer is YES - here's the PR: #2065 |
Description
Fixes: openedx/wg-build-test-release#466
Useful information to include:
Screen Recording 2025-05-29 at 12.00.01 PM
Testing instructions
Go to Section > Subsection > Unit (Text)
Click on the "text" button in the Add Components panel
Text editor opens in a modal
Click on the text editor HTML functions
Other information
The CodeEditor hook
createCodeMirrorDomNodeis doing a lookup onCODEMIRROR_LANGUAGESbut we never set thelangon the component. Since this is referenced in the project to be specifically an HTML editor, I hardcoded the "html" string to the prop for the lookup to succeed. Otherwise, there is a new change on master that includes the default as xml() (this wasn't included in the current teak release).See default here: https://github.com/tonybusa/frontend-app-authoring/blob/html-button-in-text-component-editor/src/editors/sharedComponents/CodeEditor/hooks.js#L103