-
Notifications
You must be signed in to change notification settings - Fork 6
Fix bugs in GrapesJS: invalid SVG xmlns, class mismatch, orphan tags, style syntax #22
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: phuocle <[email protected]>
…HTML tag issues, CSS style syntax, and view name separator Co-authored-by: phuocle <[email protected]>
| viewnames += `${await Crm.GetViewName(attr.entitylogicalname, viewids[i])}, `; | ||
| } | ||
| viewnames = viewnames.substring(0, viewnames.length - 1); | ||
| viewnames = viewnames.substring(0, viewnames.length - 2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why -2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The -2 is needed because I changed the separator from nothing to , (comma + space = 2 characters).
Original code:
viewnames += `${await Crm.GetViewName(...)}`; // no separator
viewnames.substring(0, viewnames.length - 1); // removes last char of last name (bug)My fix:
viewnames += `${await Crm.GetViewName(...)}, `; // adds ", " separator
viewnames.substring(0, viewnames.length - 2); // removes trailing ", "The original -1 was actually a bug since no trailing character was being added - it would incorrectly truncate the last view name. If you'd prefer to keep the original behavior without a separator, I can revert this change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This pull request fixes several bugs in the GrapesJS component that were affecting component rendering, SVG display, and string formatting. The changes address invalid SVG namespace URLs, incorrect JavaScript style syntax, a component class name mismatch, orphan HTML closing tags, and improper string concatenation for multiple view names.
Key Changes
- Fixed invalid SVG namespace URLs by correcting
xmlns="http =//..."toxmlns="http://..."in four SVG icon constants - Corrected JavaScript style property assignments by removing trailing semicolons from string values
- Fixed component detection bug by aligning the class name used in the component definition with the isComponent check
- Removed orphan
</span>closing tags that had no matching opening tags in three component files - Improved view name concatenation by using comma-space separator instead of semicolon/nothing and adjusting substring offset accordingly
Reviewed changes
Copilot reviewed 9 out of 10 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| DataverseDialogBuilder.GrapesJS/src/const.js | Fixed malformed xmlns attributes in IconLock, IconEvent, PcfIcon, and IconClick SVG constants |
| DataverseDialogBuilder.GrapesJS/src/traits.js | Removed invalid trailing semicolons from backgroundColor and color style property values |
| DataverseDialogBuilder.GrapesJS/src/components/control/richtextbox.js | Changed class name from 'DDBRichTextBox' to 'DDBControlRichTextBox' to match isComponent check |
| DataverseDialogBuilder.GrapesJS/src/components/control/label.js | Removed orphan </span> closing tag in innerHTML assignment |
| DataverseDialogBuilder.GrapesJS/src/components/control/button.js | Removed orphan </span> closing tag in innerHTML assignment |
| DataverseDialogBuilder.GrapesJS/src/components/control/optionset.js | Removed orphan </span> closing tag in innerHTML assignment |
| DataverseDialogBuilder.GrapesJS/src/components/control/subgrid.js | Added comma-space separator for view names and adjusted substring offset from -1 to -2 |
| DataverseDialogBuilder.GrapesJS/src/components/control/chart.js | Changed view name separator from semicolon to comma-space and adjusted substring offset from -1 to -2 |
| DataverseDialogBuilder.GrapesJS/package.json | Version bump from 1.0.661 to 1.0.664 |
| DataverseDialogBuilder.GrapesJS/package-lock.json | Version bump from 1.0.661 to 1.0.664 |
Files not reviewed (1)
- DataverseDialogBuilder.GrapesJS/package-lock.json: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@copilot approved
|
Code review of the GrapesJS project identified several bugs affecting component rendering and styling.
Fixed Issues
const.js):xmlns="http =//..."→xmlns="http://..."in IconLock, IconEvent, PcfIcon, IconClick constantsrichtextbox.js): Component declaredDDBRichTextBoxbut isComponent checks forDDBControlRichTextBoxlabel.js,button.js,optionset.js): Removed stray</span>tags with no matching openertraits.js):style.backgroundColor = 'gray;'→'gray'(semicolons invalid in JS style values)subgrid.js,chart.js): Multiple view names concatenated without delimiter; added,separatorExample
Before:
After:
Original prompt
✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.