fix: width bug with ad and remove old ad code#4382
Conversation
WalkthroughThe changes remove the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~7 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
CodeRabbit Configuration File (
|
![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)
src/views/DfmAdLarge.vue (1)
8-13: Move DOM-injection to a lifecycle hook to prevent SSR / timing issuesThe script tag is injected at module-eval time, which runs even during server-side rendering or unit tests where
documentis undefined. Relocate the logic tomounted()so it executes only in the browser and after the component is in the DOM:export default {}; -if (!document.getElementById('ad_script')) { - const s = document.createElement('script'); - s.id = 'ad_script'; - s.src = 'https://static.admysports.com/hb/site/camptocamp/hb_camptocamp.js'; - document.getElementsByTagName('head')[0].appendChild(s); -} +export default { + mounted() { + if (!document.getElementById('ad_script')) { + const s = document.createElement('script'); + s.id = 'ad_script'; + s.src = + 'https://static.admysports.com/hb/site/camptocamp/hb_camptocamp.js'; + document.head.appendChild(s); + } + }, +};This also avoids repeated DOM walks for every import.
🧹 Nitpick comments (1)
src/views/SideMenu.vue (1)
195-201: Delete now-unused.menu-adstyle blockThe template no longer renders any element with
menu-ad, so these lines are dead CSS:.menu-ad { margin-bottom: 15px !important; line-height: 1; }Removing them (and any associated variables) simplifies maintenance and prevents future confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/views/Advertisement.vue(0 hunks)src/views/DfmAdLarge.vue(1 hunks)src/views/SideMenu.vue(1 hunks)
💤 Files with no reviewable changes (1)
- src/views/Advertisement.vue
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build (lts/*)
- GitHub Check: CodeQL-Build
🔇 Additional comments (1)
src/views/SideMenu.vue (1)
105-107: Remove dead component registration
Advertisementwas deleted, yetcomponents: { IconYeti }still contains no reference to it—good. Double-check that no other deleted ad-related imports or mixins linger ($screen.hasHeightForAd, etc.) to keep bundle size minimal.
| #camptocamp_htad { | ||
| min-width: 728px; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hard-coding a desktop-only min-width
min-width: 728px forces horizontal scrolling or clipping on viewports narrower than 728 px (e.g. tablets in portrait, small laptop split-screen). Consider constraining the rule with a media query or falling back gracefully:
+#camptocamp_htad {
- min-width: 728px;
+ min-width: 728px; /* full leaderboard */
+ max-width: 100%; /* never overflow container */
+}
+
+@media (max-width: 767px) {
+ #camptocamp_htad {
+ min-width: auto; /* let small screens shrink */
+ }
}This keeps the fix for larger screens while avoiding layout breakage on smaller ones.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/views/DfmAdLarge.vue around lines 17 to 19, the CSS rule sets a fixed
min-width of 728px which causes layout issues on smaller viewports. To fix this,
wrap the min-width: 728px rule inside a media query targeting larger screens
only, so smaller screens do not get forced to that width and avoid horizontal
scrolling or clipping.
There was a problem hiding this comment.
The DfmAdLarge component is used only on large screen (widescreen mini so >= 1216 px)
There was a problem hiding this comment.
@flob38 Thank you for the clarification! That makes perfect sense. Since the DfmAdLarge component is only rendered on screens >= 1216px, the min-width: 728px is appropriate and won't cause any layout issues on smaller viewports. My previous suggestion about adding responsive behavior was based on the assumption that this component could be used on smaller screens, but your explanation clarifies that it's specifically designed for large screens only.
The fix directly addresses the width bug mentioned in the PR objectives without any risk of causing overflow or horizontal scrolling issues.
✏️ Learnings added
Learnt from: flob38
PR: c2corg/c2c_ui#4382
File: src/views/DfmAdLarge.vue:17-19
Timestamp: 2025-08-02T19:55:04.798Z
Learning: The DfmAdLarge component in src/views/DfmAdLarge.vue is only used on large screens (width >= 1216px), so fixed min-width values are appropriate and won't cause layout issues on smaller viewports.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
2 participants
When screen > desktop but not large enough, the ad was cropped.
And delete Advertisement.vue and code in SideMenu.vue
Summary by CodeRabbit
Bug Fixes
Style
Chores