Fix alerts bug when using non-full-width position #19
Conversation
| margin-top: 6px; | ||
| } | ||
|
|
||
| .MMM-MBTA .alerts { | ||
| overflow: hidden; | ||
| max-width: 300px; |
There was a problem hiding this comment.
300px felt like a good max-width
MMM-MBTA.js
Outdated
| alertHeader.innerHTML = "Alerts"; | ||
| wrapper.appendChild(alertHeader); | ||
|
|
||
| var alertTable = document.createElement("table"); | ||
| alertTable.className = "small"; | ||
| var alertTable = document.createElement("div"); |
There was a problem hiding this comment.
Are tables common with MagicMirror modules? Got rid of it as it can cause annoying css behavior.
There was a problem hiding this comment.
Not sure. It's been so long since I've written this. I'll just take your word for it, and it was probably the primary reason it's been so buggy as well?
|
Need to check CSS is good when more than 1 alert. |
| var row = document.createElement("tr"); | ||
| alertTable.appendChild(row); | ||
| alertTable.style.cssText = "width: inherit"; | ||
| if (hasAlerts) { |
There was a problem hiding this comment.
No need for else since length checks are inverse of each other.
MMM-MBTA.js
Outdated
| alertHeader.className = "module-header alerts-header"; | ||
| alertHeader.innerHTML = "Alerts"; | ||
| wrapper.appendChild(alertHeader); | ||
|
|
||
| var alertTable = document.createElement("table"); | ||
| alertTable.className = "small"; | ||
| var alertsDiv = document.createElement("div"); | ||
| alertsDiv.className = "alerts small"; | ||
|
|
||
| if (showEmptyAlerts) { | ||
| var alertsWrapper = document.createElement("div"); | ||
| alertsWrapper.className = "alerts-wrapper"; | ||
| alertsDiv.appendChild(alertsWrapper); | ||
|
|
||
| for (let alert of uniqueAlerts) { | ||
| var alertText = alert; | ||
| var alertDiv = document.createElement("div"); | ||
| alertDiv.innerHTML = "No alerts"; | ||
| alertDiv.className = "light small alert"; | ||
| alertsWrapper.appendChild(alertDiv); | ||
| } |
There was a problem hiding this comment.
Moved common dom updates so it's easier to discern and make changes
| /* start the alert off screen */ | ||
| padding-left: 100%; | ||
| transform: translateX(0%); | ||
| animation: alert calc(0.075s * var(--char-count, 100)) linear infinite; |
There was a problem hiding this comment.
Set a CSS variable in JS with the number of characters the alerts are and use it in the animation timing.
|
Hey @cronin4392, thank you so much for the pull request. It means a lot to me to see this still get some love and care. Unfortunately I don't have the dev environment for a Magic mirror right now, so reviewing this code is a little tricky for me right now. Your changes look solid though from a review standpoint, so I'm inclined to merge. Do you mind providing some before/after images? I'd really like to see the effects of this change visually, just to add a little more confident in merging. |
|
Hey @edward-shen sorry for the delay, lost track of this and didn't see your comment until now! Before: After: |
|
Thanks! Merged. |
Fixes #1.
<table>.max-widtharound the alerts.