Cus-479-new UI is breaking in widget and dashboard if any msg have table component #1306
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webplugin/js/app/mck-sidebox-1.0.js (1)
8760-8780: Styling implementation looks good, with minor considerations.The addition of styles to handle tables in rich messages addresses the breaking UI issue effectively. The defensive check for
kmElementand its shadow root before appending styles is good practice.Some considerations:
The implementation doesn't include specific styling for table headers (
th). If table headers are used in your messages, consider adding specific styling for them.The hard-coded colors (
#EFEFEF,#D0D5DD,#535862) might not adapt if your application has a theming system or dark mode.If your application supports themes or dark mode, consider using CSS variables for colors:
style.textContent = ` table { - background-color: #EFEFEF; - border: 1px solid #D0D5DD; + background-color: var(--table-bg-color, #EFEFEF); + border: 1px solid var(--table-border-color, #D0D5DD); /* other properties */ - color: #535862; + color: var(--table-text-color, #535862); } td { padding: 15px; - border: 1px solid #D0D5DD; + border: 1px solid var(--table-border-color, #D0D5DD); /* other properties */ } `;webplugin/scss/components/_km-message-area.scss (1)
112-114: Enhance Overflow Handling & Clipping for Message TablesThe new styles (
overflow-x: auto;,white-space: nowrap;, andclip-path: inset(0px round 24px);) ensure that when a message includes a wide table component, the content will scroll horizontally rather than breaking the layout. This appears to directly address the UI breakage issue mentioned in the PR.
- Browser Compatibility Check: Since Internet Explorer is among the targeted browsers for testing, verify that these CSS properties, particularly
clip-path, work as intended on IE. You may need to consider a fallback or a vendor-prefixed variant (e.g.,-webkit-clip-path) for browsers with limited support.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
webplugin/js/app/mck-sidebox-1.0.js(1 hunks)webplugin/scss/components/_km-message-area.scss(1 hunks)
webplugin/js/app/mck-sidebox-1.0.js
Outdated
| const style = document.createElement('style'); | ||
| style.textContent = ` | ||
| table { | ||
| border-collapse: separate; | ||
| border-spacing: 0; | ||
| background-color: #EFEFEF; | ||
| overflow: hidden; | ||
| border: 1px solid #D0D5DD; | ||
| border-radius: 15px; | ||
| font-weight: 300; | ||
| color: #535862; | ||
| } | ||
| td { | ||
| padding: 15px; | ||
| border: 1px solid #D0D5DD; | ||
| text-align: center; | ||
| } | ||
| `; | ||
| kmElement.shadowRoot.appendChild(style); |
There was a problem hiding this comment.
@KomMohit define it inside the custom element class
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
webplugin/js/app/mck-sidebox-1.0.js (1)
8759-8759: Correct fix for preserving existing content in shadow DOM.Changing from
=to+=ensures that emoji templates are appended to existing content in the shadow DOM rather than overwriting it. This directly addresses the issue where UI was breaking when messages contained table components, as the previous implementation was replacing the entire shadow DOM content including any tables.While this fix works, consider refactoring this functionality into the custom element class itself as suggested in a previous review. This would improve encapsulation and maintainability by keeping the shadow DOM manipulation logic within the component that owns it.
.husky/.gitignore (1)
1-1: Clarify the intent of the underscore (_) entry.The file currently ignores items matching the literal underscore. It isn’t immediately clear what specific files or directories this is meant to exclude within the
.huskycontext. Please consider adding an inline comment above the underscore to provide clarity on its purpose, ensuring future maintainers understand its intent.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.husky/.gitignore(1 hunks).husky/pre-commit(0 hunks)package.json(1 hunks)webplugin/js/app/components/custom-element.js(1 hunks)webplugin/js/app/mck-sidebox-1.0.js(1 hunks)webplugin/scss/components/_km-message-area.scss(1 hunks)
💤 Files with no reviewable changes (1)
- .husky/pre-commit
🔇 Additional comments (3)
webplugin/scss/components/_km-message-area.scss (1)
112-114: Good fix for table overflow issues in message boxesThese CSS changes address the issue where tables were breaking the UI layout. The combination of
overflow-x: autoandwhite-space: nowrapallows tables to be horizontally scrollable rather than overflowing, while theclip-pathensures consistent rounded corners for the container.webplugin/js/app/components/custom-element.js (1)
5-24: Well-structured table styling for the shadow DOMThe addition of these table styles to the shadow DOM is crucial for fixing the reported issue. The styles provide consistent formatting for tables embedded within messages, preventing them from breaking the UI layout.
A few observations:
- The table styling uses a clean, modern approach with appropriate spacing and borders
- Proper handling of border-radius and overflow ensures tables look consistent with the rest of the UI
- The styling maintains readability with appropriate padding and text alignment
package.json (1)
78-78: Husky dependency upgrade appears valid.The Husky version in
devDependencieshas been updated from "^7.0.0" to "^7.0.4". This minor patch upgrade should bring improvements or bug fixes without breaking existing functionality. Please verify that all Git hook scripts (especially in light of the removal of the.husky/pre-commitfile) continue to work as expected with the new version.
This reverts commit a01c5f6.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
webplugin/js/app/mck-sidebox-1.0.js (3)
7987-7990: Remove console.log statement.The console.log statement on line 7989 appears to be debugging code that should be removed before production deployment.
if (element) { - console.log('deleted'); element.remove(); }
7997-7999: Consider adding a comment explaining why the text box is made editable.The code sets the text box as editable when a tokenized message is received, but it's not clear why this is necessary.
if (msg.tokenMessage && !msgThroughListAPI) { // message not from the sockets + // Make text box editable when tokenized message is received to allow user interaction document.getElementById('mck-text-box').setAttribute('contenteditable', true); }
8741-8741: Watch out for cumulative innerHTML assignment.Using
+=with innerHTML appends content rather than replacing it. This might cause duplication if this code runs multiple times.- kmElement._shadow.innerHTML += emoji_template; + kmElement._shadow.innerHTML = emoji_template;If you do need to append rather than replace, consider checking if this is intentional and documented.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
webplugin/js/app/components/custom-element.js(1 hunks)webplugin/js/app/mck-sidebox-1.0.js(8 hunks)webplugin/scss/components/_km-message-area.scss(1 hunks)
🔇 Additional comments (8)
webplugin/js/app/components/custom-element.js (1)
5-23: Good addition of table styling in ShadowDomComponentThe added styling for tables and cells provides structure and consistent appearance for table components within the shadow DOM. This addresses the UI breaking issue mentioned in the PR title.
Some suggestions for consideration:
- Consider extracting these styles to a separate CSS file that could be imported, which would improve maintainability.
- Adding responsive styling for small screens might be beneficial.
webplugin/scss/components/_km-message-area.scss (1)
112-114: Good overflow handling for message boxesThese CSS properties are essential to fix the issue mentioned in the PR title:
overflow-x: autoenables horizontal scrolling for wide content like tableswhite-space: nowrapprevents content from wrapping and potentially breaking layoutsclip-path: inset(0px round 24px)ensures rounded corners match the container's styleThese changes align well with the table styling added in the custom-element.js file.
webplugin/js/app/mck-sidebox-1.0.js (6)
8211-8214: Code logic looks good.The additional conditions for bot message delay handling correctly ensure that tokenized responses from bots are handled appropriately.
8711-8712: Code looks good for handling tokenized messages.The condition correctly identifies tokenized messages from the left side that didn't come through the list API and applies special processing.
10828-10829: The condition for tokenized messages looks good.The additional check for tokenized messages with content type other than 5 appears to be correct and consistent with the changes in other parts of the file.
10908-10910: Consistent implementation of tokenization check.This change consistently applies the same pattern seen elsewhere in the code, checking for bot messages and tokenization status.
14273-14275: Consistent implementation of bot message delay check.This change follows the same pattern as other sections, ensuring bot message delays are skipped when tokenization is enabled.
8741-8742:✅ Verification successful
Be cautious when directly manipulating shadow DOM content.
Directly accessing and modifying
_shadow.innerHTMLcould be risky as properties prefixed with underscore are typically considered private.Consider whether there's an established pattern or API for updating the shadow DOM content in this codebase. You might want to check:
🏁 Script executed:
#!/bin/bash # Search for other instances of _shadow modifications to verify this pattern rg "_shadow\.innerHTML" --type jsLength of output: 246
Shadow DOM Modification Pattern Confirmed
The direct manipulation of
_shadow.innerHTMLinmck-sidebox-1.0.jsis consistent with the pattern observed in other parts of the code—specifically, the similar assignment injquery.linkify.js. While using properties prefixed with an underscore does signal that the API is private and could be risky, this approach appears to be an intentional design choice across the codebase.
- Action: Ensure that the use of this private API is thoroughly documented and revisit if a public API or helper function becomes available.
What do you want to achieve?
PR Checklist
How was the code tested?
What new thing you came across while writing this code?
In case you fixed a bug then please describe the root cause of it?
Screenshot
NOTE: Make sure you're comparing your branch with the correct base branch
Summary by CodeRabbit
Style
Bug Fixes