fix: correct X (Twitter) icon display and navbar color behavior on scroll #903
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- added new x-logo.svg - fixed header icon color swap (white → black on scroll) - ensured footer icon stays black - updated custom.css and scripts.js to properly handle invert + scroll classes - updated all HTML files to reference the SVG correctly Closes fossasia#902
Reviewer's GuideUpdates Twitter/X social icon usage and implements scroll-based navbar color behavior by introducing a reusable SVG icon, centralized CSS filter rules for different layout regions, and JavaScript scroll handling that toggles navbar state instead of forcing it during load. Sequence diagram for scroll-based navbar show_content state and X icon colorsequenceDiagram
actor User
participant Browser
participant Document
participant Window
participant Nav as overlay_nav
participant CSS as custom_css
User->>Browser: Load page
Browser->>Document: Fire DOMContentLoaded
Document->>Nav: querySelector overlay_nav
Document->>Nav: remove show_content
Nav->>CSS: social-icon in nav uses invert(1) (white at top)
User->>Window: Scroll page
Window->>Window: scroll event (> 50)
Window->>Nav: add show_content
Nav->>CSS: nav .social-icon uses invert(0) (black when scrolled)
User->>Window: Scroll back to top
Window->>Window: scroll event (<= 50)
Window->>Nav: remove show_content
Nav->>CSS: nav .social-icon uses invert(1) (white at top)
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- The new
DOMContentLoadedhandler assumes.overlay-navalways exists; consider null-checkingnavbefore callingclassList.remove/addto avoid runtime errors on pages without that element. - The CSS for
.social-iconrelies on multiple!importantoverrides for different containers; it might be more maintainable to reduce!importantusage by tightening selectors or using modifier classes to express the state instead. - Now that
$('nav').addClass('show-content')is commented out with an inline note, consider either removing it or replacing it with a conditional that respects the scroll logic, so the behavior is encoded in code rather than in comments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `DOMContentLoaded` handler assumes `.overlay-nav` always exists; consider null-checking `nav` before calling `classList.remove`/`add` to avoid runtime errors on pages without that element.
- The CSS for `.social-icon` relies on multiple `!important` overrides for different containers; it might be more maintainable to reduce `!important` usage by tightening selectors or using modifier classes to express the state instead.
- Now that `$('nav').addClass('show-content')` is commented out with an inline note, consider either removing it or replacing it with a conditional that respects the scroll logic, so the behavior is encoded in code rather than in comments.
## Individual Comments
### Comment 1
<location> `js/scripts.js:387-390` </location>
<code_context>
+
+// Force header to start in "not scrolled" state
+document.addEventListener("DOMContentLoaded", function () {
+ const nav = document.querySelector(".overlay-nav");
+ nav.classList.remove("show-content"); // remove black mode at start
+
+ window.addEventListener("scroll", function () {
+ if (window.scrollY > 50) {
+ nav.classList.add("show-content"); // enable black on scroll
</code_context>
<issue_to_address>
**issue (bug_risk):** Guard against missing `.overlay-nav` to avoid runtime errors on pages without that element.
On pages without `.overlay-nav`, `document.querySelector` returns `null`, so `nav.classList.remove` will throw and stop subsequent JS from running. Please add a null check (e.g. `if (!nav) return;`) after the query to make this safe.
</issue_to_address>
### Comment 2
<location> `index.html:881-882` </location>
<code_context>
<span class="uppercase">Oh, invite your friends too</span>
<a target="_blank" href="https://www.facebook.com/fossasia/"><i class="icon social_facebook icon-large"></i></a>
- <a target="_blank" href="https://twitter.com/fossasia"><i class="icon social_twitter"></i></a>
+ <a target="_blank" href="https://twitter.com/fossasia"><img src="img/x-logo.svg" alt="X logo" class="social-icon">
+</a>
<a target="_blank" href="https://de.linkedin.com/company/fossasia"><i class="icon social_linkedin icon-large"></i></a>
</div>
</code_context>
<issue_to_address>
**suggestion:** Clarify the `alt` text to better convey the destination/action rather than just the logo name.
Using just "X logo" as the `alt` text doesn’t convey the purpose of the link to screen reader users. Consider something like "FOSSASIA on X (Twitter)" or, for share buttons, "Share on X (Twitter)" so the action/destination is clear.
Suggested implementation:
```
<li><a href="https://twitter.com/fossasia" target="_self"><img src="img/x-logo.svg" alt="FOSSASIA on X (Twitter)" class="social-icon">
</a></li>
```
```
<a target="_blank" href="https://twitter.com/fossasia"><img src="img/x-logo.svg" alt="FOSSASIA on X (Twitter)" class="social-icon">
</a>
```
```
<img src="img/x-logo.svg" alt="FOSSASIA on X (Twitter) feed" class="social-icon">
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this PR do?
fixes the X (Twitter) icon not appearing and resolves incorrect color behavior in the navbar during scroll.
Changes Made
img/x-logo.svgcustom.cssto handle invert logic for navbar vs footerscripts.jsto remove forcedshow-contentflash and apply proper scroll logicScreenshots
Please find the before/after screenshots and optional demo video below:
Before:
X icon missing in header
After:
X icon visible and styled correctly
Smooth color transition on scroll
Screen.Recording.2025-12-03.020414.mp4
Related Issue
Closes #902
Tested On
All pages show the correct behavior now.
Summary by Sourcery
Update social media icon assets and scroll behavior so the X (Twitter) logo displays consistently and navbar icon colors change correctly on scroll across site pages.
New Features:
Bug Fixes:
Enhancements: