Skip to content

fix: width bug with ad and remove old ad code #4382

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.

Sign up for GitHub

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

Merged
brunobesson merged 1 commit into from
Aug 4, 2025
+7 −71
Merged

fix: width bug with ad and remove old ad code #4382

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 0 additions & 61 deletions src/views/Advertisement.vue
View file
Open in desktop

This file was deleted.

6 changes: 6 additions & 0 deletions src/views/DfmAdLarge.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,3 +12,9 @@ if (!document.getElementById('ad_script')) {
document.getElementsByTagName('head')[0].appendChild(s);
}
</script>

<style lang="css" scoped>
#camptocamp_htad {
min-width: 728px;
}
Comment on lines +17 to +19

@coderabbitai coderabbitai Bot Aug 2, 2025 •
edited
Loading

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

@flob38 flob38 Aug 2, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The DfmAdLarge component is used only on large screen (widescreen mini so >= 1216 px)

@coderabbitai coderabbitai Bot Aug 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@flob38 flob38 Aug 2, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@brunobesson to check the rabbit ;)

@coderabbitai coderabbitai Bot Aug 2, 2025

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

</style>
11 changes: 1 addition & 10 deletions src/views/SideMenu.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@
</router-link>

<div class="menu-footer is-size-7">
<advertisement class="menu-add" v-if="$screen.hasHeightForAd" />
<div class="has-text-centered menu-links">
<router-link :to="{ name: 'article', params: { id: 106727 } }" v-translate>contact</router-link>
<span> &bull; </span>
Expand Down Expand Up @@ -101,12 +100,10 @@
</template>

<script>
import Advertisement from './Advertisement';

import IconYeti from '@/components/generics/icons/IconYeti';

export default {
components: { IconYeti, Advertisement },
components: { IconYeti },

data() {
return {
Expand Down Expand Up @@ -202,12 +199,6 @@ aside {
line-height: 1;
}

.menu-ad {
height: 320px;
margin-left: calc((200px - 160px) / 2);
margin-right: calc((200px - 160px) / 2);
}

.instagram-icon,
.facebook-icon,
.donate-icon,
Expand Down
Loading