-
Notifications
You must be signed in to change notification settings - Fork 46
Fix "do-not-translate" and link are mutally exclusive #4023
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: develop
Are you sure you want to change the base?
Fix "do-not-translate" and link are mutally exclusive #4023
Conversation
PeterNerlich
left a comment
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.
Sorry, but this makes matters much worse:
Write a paragraph, select something in it and mark it with do not translate → the whole paragraph turns do not translate instead of only the selection
|
@PeterNerlich I hope the new approach is doing fine 🙈 |
| en: Fix the bug that "do-not-translate" and link are mutally exclusive | ||
| de: Behebe den Fehler, dass "do-not-translate" und link sich gegenseitig ausschließen |
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.
| en: Fix the bug that "do-not-translate" and link are mutally exclusive | |
| de: Behebe den Fehler, dass "do-not-translate" und link sich gegenseitig ausschließen | |
| en: Fix the bug that "do-not-translate" and a link are mutally exclusive | |
| de: Behebe den Fehler, dass "do-not-translate" und eine Verlinkung sich gegenseitig ausschließen |
| if (!anchor) { | ||
| editor.insertContent( | ||
| `<a href=${realUrl}${autoupdate ? ' data-integreat-auto-update="true"' : ""}>${text}</a>` | ||
| let selectedText = editor.selection.getNode().closest("span"); |
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.
What is the idea behind getting the nearest span element? Is it on purpose that one should not be able to make a smaller substring of a do not translate a link? If yes, would it make sense to make this leave other spans alone and only exhibit this behaviour for nodes actually marked as do not translate?
fa44b96 to
f57ab03
Compare
hannaseithe
left a comment
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.
Thank you for the PR 🐈⬛
I made one suggestion. But I am also wondering if there was a good reason you included .closest("span") (as Peter already asked). So if that doesn't work as I imagine, let me know.
| let selectedText = editor.selection.getNode().closest("span"); | ||
| if (!selectedText) { | ||
| selectedText = editor.selection.getContent({ format: "html" }); | ||
| } | ||
| const link = editor.dom.create( | ||
| "a", | ||
| { | ||
| href: `${realUrl}${autoupdate ? ' data-integreat-auto-update="true"' : ""}`, | ||
| }, | ||
| selectedText |
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.
| let selectedText = editor.selection.getNode().closest("span"); | |
| if (!selectedText) { | |
| selectedText = editor.selection.getContent({ format: "html" }); | |
| } | |
| const link = editor.dom.create( | |
| "a", | |
| { | |
| href: `${realUrl}${autoupdate ? ' data-integreat-auto-update="true"' : ""}`, | |
| }, | |
| selectedText | |
| let selectedNode = editor.selection.getNode(); | |
| if (!selectedNode) { | |
| selectedText = editor.selection.getContent({ format: "html" }); | |
| } | |
| const link = editor.dom.create( | |
| "a", | |
| { | |
| href: `${realUrl}${autoupdate ? ' data-integreat-auto-update="true"' : ""}`, | |
| }, | |
| selectedNode ?? selectedText |
Two reasons for this suggestion:
- Code readability: Assigning a node to a variable with the naming
selectedTextmight cause confusion - By deleting the .closest("span"), we also make sure that other elements like
<strong>do not get overwritten when creating a link ... hope that doesn't break something else though
Short description
This PR fixes the bug that
<a href>and "do-not-translate" marking are removing the other tag each other.Proposed changes
Side effects
Faithfulness to issue description and design
There are no intended deviations from the issue and design.
How to test
Resolved issues
Fixes: #4017
Pull Request Review Guidelines