feat(zmskvr-160) update conformation for export #929
Conversation
WalkthroughThe changes involve updates to the Twig template and JavaScript files to enhance user interaction with copy functionality. The Changes
Sequence Diagram(s)Appointment Copy FlowsequenceDiagram
participant U as User
participant B as Copy Button (Appointment)
participant V as Appointment View
U->>B: Clicks copy button
B->>V: Event triggers onCopyButton (from options)
V->>V: Executes copy callback
Workstation Copy FlowsequenceDiagram
participant U as User
participant B as Copy Button (Workstation)
participant V as Workstation View
participant D as Document
U->>B: Clicks copy button
B->>V: Triggers onCopyButton method
V->>V: Prevents default and creates text selection
V->>D: Executes navigator.clipboard.writeText
D-->>V: Returns success or failure
V->>V: Clears selection and alerts result
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Note 🎁 Summarized by CodeRabbit FreeYour organization has reached its limit of developer seats under the Pro Plan. For new users, CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please add seats to your subscription by visiting https://app.coderabbit.ai/login.If you believe this is a mistake and have available seats, please assign one to the pull request author through the subscription management page using the link above. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
zmsadmin/templates/element/helper/messages.twig (1)
303-303:⚠️ Potential issueCopy button missing corresponding content ID
The copy button has ID 'copy-popup-content', but the JavaScript implementation in workstation/index.js looks for an element with ID 'popup-content' to copy, which doesn't exist in this template.
There are two ways to fix this:
- Add an ID to the content that should be copied:
-<dl> +<dl id="popup-content">
- Or, as suggested in the previous review comment, modify the JavaScript to use a class selector instead of relying on a specific ID.
🧹 Nitpick comments (1)
zmsadmin/js/block/appointment/index.js (1)
173-174: Add error handling for undefined callbackThe event handler calls
this.onCopyButtonwithout checking if it exists, which could cause a runtime error if the callback wasn't provided in the options.}).on('click', '.form-actions .copy-btn', (event) => { - this.onCopyButton(this, event); + if (typeof this.onCopyButton === 'function') { + this.onCopyButton(this, event); + } }).on('click', '.form-actions button.process-delete', (event) => {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
zmsadmin/templates/element/helper/messages.twig(2 hunks)zmsadmin/js/block/appointment/index.js(2 hunks)zmsadmin/js/page/workstation/index.js(3 hunks)zmsadmin/templates/element/helper/messages.twig(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: call-php-unit-tests / zmsdb-test
- GitHub Check: call-php-code-quality / module-code-quality (zmsdldb, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmsdb, 8.0)
- GitHub Check: call-php-code-quality / module-code-quality (zmsclient, 8.0)
- GitHub Check: call-php-unit-tests / zmsapi-test
🔇 Additional comments (5)
zmsadmin/js/block/appointment/index.js (1)
59-59: Added callback for copy button functionalityThis addition sets up a new callback for copy button functionality, which aligns with the changes in other files.
zmsadmin/js/page/workstation/index.js (2)
54-54: Method binding for copy button functionalityCorrectly added the new method to the bindPublicMethods call.
617-617: Correctly added copy button handler to AppointmentView optionsThis line properly passes the onCopyButton callback to the AppointmentView component.
zmsadmin/templates/element/helper/messages.twig (2)
237-240: Improved display of appointment informationThe new template code uses Twig's reduce, map, and join filters to create a more concise display of request information. This is a more elegant solution than previous verbose implementations.
296-299: Consistent formatting for appointment informationThe same pattern has been consistently applied here as in the previous block, which is good for maintaining consistency across the UI.
| onCopyButton(event) { | ||
| stopEvent(event); | ||
| console.log("Hallo"); | ||
| document.addEventListener('DOMContentLoaded', function () { | ||
| var copyButton = document.getElementById('copy-popup-content'); | ||
| var popupContent = document.getElementById('popup-content'); | ||
|
|
||
| copyButton.addEventListener('click', function () { | ||
| var range = document.createRange(); | ||
| range.selectNode(popupContent); | ||
| window.getSelection().removeAllRanges(); | ||
| window.getSelection().addRange(range); | ||
|
|
||
| try { | ||
| var successful = document.execCommand('copy'); | ||
| var msg = successful ? 'erfolgreich' : 'nicht erfolgreich'; | ||
| alert('Inhalt wurde ' + msg + ' kopiert'); | ||
| } catch (err) { | ||
| alert('Kopieren war nicht erfolgreich'); | ||
| } | ||
|
|
||
| window.getSelection().removeAllRanges(); | ||
| }); | ||
| }); | ||
|
|
||
| } |
There was a problem hiding this comment.
Fix implementation of copy functionality
The current implementation has several critical issues:
- The
DOMContentLoadedevent listener is added inside the click handler, which is ineffective as this event only fires once when the document initially loads. - Console log statement "Hallo" appears to be debugging code.
- The code adds new event listeners each time the button is clicked, which could cause memory leaks.
- There's no check to ensure the required DOM elements exist.
Replace with a more straightforward and reliable implementation:
onCopyButton(event) {
stopEvent(event);
- console.log("Hallo");
- document.addEventListener('DOMContentLoaded', function () {
- var copyButton = document.getElementById('copy-popup-content');
- var popupContent = document.getElementById('popup-content');
-
- copyButton.addEventListener('click', function () {
- var range = document.createRange();
- range.selectNode(popupContent);
- window.getSelection().removeAllRanges();
- window.getSelection().addRange(range);
-
- try {
- var successful = document.execCommand('copy');
- var msg = successful ? 'erfolgreich' : 'nicht erfolgreich';
- alert('Inhalt wurde ' + msg + ' kopiert');
- } catch (err) {
- alert('Kopieren war nicht erfolgreich');
- }
-
- window.getSelection().removeAllRanges();
- });
- });
+ const popupContent = document.querySelector('.message__body dl');
+
+ if (!popupContent) {
+ console.error('Cannot find content to copy');
+ return;
+ }
+
+ try {
+ const range = document.createRange();
+ range.selectNode(popupContent);
+ window.getSelection().removeAllRanges();
+ window.getSelection().addRange(range);
+
+ const successful = document.execCommand('copy');
+ const msg = successful ? 'erfolgreich' : 'nicht erfolgreich';
+
+ // Use a more user-friendly notification instead of alert
+ const messageContainer = $(event.currentTarget).closest('.message').find('.message__body');
+ const notification = $('<p class="copy-notification"></p>').text('Inhalt wurde ' + msg + ' kopiert');
+ messageContainer.append(notification);
+
+ // Remove notification after a delay
+ setTimeout(() => {
+ notification.fadeOut(() => notification.remove());
+ }, 3000);
+ } catch (err) {
+ console.error('Copy failed:', err);
+ // Show error notification
+ const messageContainer = $(event.currentTarget).closest('.message').find('.message__body');
+ const notification = $('<p class="copy-notification error"></p>').text('Kopieren war nicht erfolgreich');
+ messageContainer.append(notification);
+
+ // Remove notification after a delay
+ setTimeout(() => {
+ notification.fadeOut(() => notification.remove());
+ }, 3000);
+ }
+
+ window.getSelection().removeAllRanges();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onCopyButton(event) { | |
| stopEvent(event); | |
| console.log("Hallo"); | |
| document.addEventListener('DOMContentLoaded', function () { | |
| var copyButton = document.getElementById('copy-popup-content'); | |
| var popupContent = document.getElementById('popup-content'); | |
| copyButton.addEventListener('click', function () { | |
| var range = document.createRange(); | |
| range.selectNode(popupContent); | |
| window.getSelection().removeAllRanges(); | |
| window.getSelection().addRange(range); | |
| try { | |
| var successful = document.execCommand('copy'); | |
| var msg = successful ? 'erfolgreich' : 'nicht erfolgreich'; | |
| alert('Inhalt wurde ' + msg + ' kopiert'); | |
| } catch (err) { | |
| alert('Kopieren war nicht erfolgreich'); | |
| } | |
| window.getSelection().removeAllRanges(); | |
| }); | |
| }); | |
| } | |
| onCopyButton(event) { | |
| stopEvent(event); | |
| const popupContent = document.querySelector('.message__body dl'); | |
| if (!popupContent) { | |
| console.error('Cannot find content to copy'); | |
| return; | |
| } | |
| try { | |
| const range = document.createRange(); | |
| range.selectNode(popupContent); | |
| window.getSelection().removeAllRanges(); | |
| window.getSelection().addRange(range); | |
| const successful = document.execCommand('copy'); | |
| const msg = successful ? 'erfolgreich' : 'nicht erfolgreich'; | |
| // Use a more user-friendly notification instead of alert | |
| const messageContainer = $(event.currentTarget).closest('.message').find('.message__body'); | |
| const notification = $('<p class="copy-notification"></p>').text('Inhalt wurde ' + msg + ' kopiert'); | |
| messageContainer.append(notification); | |
| // Remove notification after a delay | |
| setTimeout(() => { | |
| notification.fadeOut(() => notification.remove()); | |
| }, 3000); | |
| } catch (err) { | |
| console.error('Copy failed:', err); | |
| // Show error notification | |
| const messageContainer = $(event.currentTarget).closest('.message').find('.message__body'); | |
| const notification = $('<p class="copy-notification error"></p>').text('Kopieren war nicht erfolgreich'); | |
| messageContainer.append(notification); | |
| // Remove notification after a delay | |
| setTimeout(() => { | |
| notification.fadeOut(() => notification.remove()); | |
| }, 3000); | |
| } | |
| window.getSelection().removeAllRanges(); | |
| } |
|
@coderabbitai The Popup should not close after click on copy button. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chainI'll help you fix the issue with the popup closing when the copy button is clicked. Let me first examine the current implementation of the copy button functionality. Let me get a better understanding of the popup implementation and the copy button functionality: I see the issue. The current Here's how to fix it: @msfb3 You need to modify the onCopyButton: function (event) {
event.preventDefault();
event.stopPropagation(); // Add this line to prevent event bubbling
var text = document.getElementById('copy-content').textContent;
navigator.clipboard.writeText(text);
}This will prevent the click event from bubbling up to parent elements that might be triggering the popup to close. 🏁 Script executed: Length of output: 68522 🏁 Script executed: Length of output: 39984 Let me look at the @msfb3 You need to modify the Add onCopyButton(event) {
stopEvent(event);
event.stopPropagation(); // Add this line to prevent event bubbling
var popupContent = document.getElementById('copy-content');
var textToCopy = popupContent.innerText || popupContent.textContent;
navigator.clipboard.writeText(textToCopy)
}This will prevent the click event from reaching any parent elements that might be triggering the popup to close. 🏁 Script executed: Length of output: 168 @msfb3 I see the issue. Looking at the export const stopEvent = (ev) => {
if (ev) {
ev.preventDefault();
ev.stopPropagation();
}
}Since event propagation is already being stopped, the issue might be that other event handlers are still being triggered on the same element. You should add onCopyButton(event) {
stopEvent(event);
event.stopImmediatePropagation(); // Add this line to prevent other handlers from executing
var popupContent = document.getElementById('copy-content');
var textToCopy = popupContent.innerText || popupContent.textContent;
navigator.clipboard.writeText(textToCopy);
}This should prevent the popup from closing when the copy button is clicked. ✏️ Learnings added
|
|
@coderabbitai stopImmediatePropagation is not working. The Popup will also close. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 65697 🏁 Script executed: Length of output: 85490 🏁 Script executed: Length of output: 67535 🏁 Script executed: Length of output: 126539 🏁 Script executed: Length of output: 67077 🏁 Script executed: Length of output: 7067 Based on the code inspection, I can see the issue. The popup is closing because the click event is propagating to parent elements. Let me explain the current implementation and propose a fix: @msfb3 The issue is that the click event is still reaching the dialog's event handlers. Here's how to fix it: onCopyButton(event) {
event.preventDefault();
event.stopImmediatePropagation(); // Prevent other handlers from executing
const button = event.currentTarget;
if (!button.closest('.dialog')) {
// Only copy if not inside dialog to prevent closing
var popupContent = document.getElementById('copy-content');
var textToCopy = popupContent.innerText || popupContent.textContent;
navigator.clipboard.writeText(textToCopy);
}
}This modification will:
Let me know if this resolves the issue for you. 🧠 Learnings used |
Pull Request Checklist (Feature Branch to
next):nextBranch in meinen Feature-Branch gemergt.Summary by CodeRabbit