Add favicon to landing page#27
Conversation
juanmanueldaza
left a comment
juanmanueldaza
left a comment
There was a problem hiding this comment.
Changes Requested — Scope Creep + LinkedIn Branding
Two issues:
-
Scope creep: This PR includes the JSON-LD fix (PR #26) and Quick Start fix (PR #28) alongside the favicon addition. Keep only the favicon changes.
-
LinkedIn brand color: The favicon SVG uses
#0077b5which is LinkedIn's brand blue. GitHub's brand colors are#24292e(dark) or#2ea44f(green). This was copy-pasted from linkedin2md.
Please: (1) Remove JSON-LD and Quick Start changes, (2) Change #0077b5 to #24292e or #2ea44f.
| "operatingSystem": "Cross-platform", | ||
| "offers": { | ||
| "@type": "Offer", | ||
| "price": "0", |
| "author": { | ||
| "@type": "Person", | ||
| "name": "Juan Manuel Daza", | ||
| "url": "https://daza.ar" |
There was a problem hiding this comment.
0x2db97e073f4a761b401b01430e9248179491
b6ae
| "offers": { | ||
| "@type": "Offer", | ||
| "price": "0", | ||
| "priceCurrency": "USD" |
| <div class="buttons"> | ||
| <a | ||
| href="https://github.com/juanmanueldaza/github2md" | ||
| class="btn btn-primary" |
There was a problem hiding this comment.
0x2db97e073f4a761b401b01430e9248179491
b6ae
| <div | ||
| class="install-box" | ||
| style="max-width: 100%; text-align: left" | ||
| > |
There was a problem hiding this comment.
0x2db97e073f4a761b401b01430e9248179491
b6ae
| <span class="prompt">$</span> pipx install | ||
| github2md<br /> | ||
| <span class="prompt">$</span> github2md | ||
| your_export.zip<br /> |
There was a problem hiding this comment.
0x2db97e073f4a761b401b01430e9248179491
b6ae
| github2md<br /> | ||
| <span class="prompt">$</span> github2md | ||
| your_export.zip<br /> | ||
| <span class="prompt">$</span> github2md export.zip -o |
There was a problem hiding this comment.
0x2db97e073f4a761b401b01430e9248179491
b6ae
|
|
||
| <footer> | ||
| <p> | ||
| Open source (GPL-2.0) · Built by |
There was a problem hiding this comment.
0x2db97e073f4a761b401b01430e9248179491
b6ae
| <footer> | ||
| <p> | ||
| Open source (GPL-2.0) · Built by | ||
| <a href="https://github.com/juanmanueldaza" |
There was a problem hiding this comment.
| <a href="https://github.com/juanmanueldaza"0x2db97e073f4a761b401b01430e9248179491 | |
| b6ae |
|
Hola @MD-Mushfiqur123 — just checking in. The review asked for:
Any updates? |
|
👋 @MD-Mushfiqur123 — quick check-in on your open PRs here:
All have review feedback about scope creep — each bundles changes from other PRs. Happy to re-review once they're scoped to their titles. Let me know if you're still working on these! |
MD-Mushfiqur123
force-pushed
the
feat/add-favicon
branch
from
acaef3b to
83e9962
Compare
|
Addressed: Stripped all unrelated changes. Now includes ONLY |
juanmanueldaza
left a comment
juanmanueldaza
left a comment
There was a problem hiding this comment.
Review Summary
Verdict: Changes Requested — one remaining item from the original review.
What was fixed ✅
- Scope creep resolved — diff now contains ONLY
docs/favicon.svg+<link rel="icon">inindex.html. Clean.
Still needed 🔴
- Favicon color still uses LinkedIn brand blue (
#0077b5). Per the original review, please change to GitHub's brand colors:#24292e(dark) or#2ea44f(green). The project isgithub2md, so GitHub-branded colors are more appropriate.
| Lens | Score | Notes |
|---|---|---|
| DRY | 10/10 | Single file addition |
| KISS | 10/10 | 4-line SVG + 1 HTML link tag |
| YAGNI | 10/10 | Directly closes #14 |
| OWASP | 10/10 | Static asset, no security concerns |
Once the color is updated, good to merge.
| @@ -0,0 +1,4 @@ | |||
| <svg xmlns="http://www.w3.org/2000/svg" viewBox="0 0 64 64"> | |||
| <rect width="64" height="64" rx="12" fill="#0077b5"/> | |||
There was a problem hiding this comment.
🔴 Color is still LinkedIn blue #0077b5. Change to #24292e (GitHub dark) or #2ea44f (GitHub green) to match the project identity.
juanmanueldaza
left a comment
juanmanueldaza
left a comment
There was a problem hiding this comment.
PR #27 Review: Changes Requested
Thanks for addressing the scope creep from the first pass — the favicon.svg is clean. However, this PR is still not ready to merge.
Problem: Still based on old commit
This branch (83e9962) is 3 commits behind current main. The diff shows changes that conflict with the analytics migration already merged into main:
- Deleting
.github/CODEOWNERS— added in PR #24. Removing it undoes recent work. - Removing README badges (CI status, "Contributions Welcome", "Good First Issues") — not related to the favicon.
- Changing GA4 measurement ID from
G-757KW9Y1RMtoG-PV6HF4TEQ7— unexplained, not related to the favicon.
What to do
- Rebase on current main —
git fetch origin && git rebase origin/main - Keep only the favicon change — the diff should show only
docs/favicon.svgadded and a minimal<link rel="icon">tag indocs/index.html. - Restore CODEOWNERS and README — these deletions should not be part of this PR.
The favicon SVG itself is good — simple, effective. Once rebased and scoped to just the favicon, this is ready to merge.
juanmanueldaza
left a comment
juanmanueldaza
left a comment
There was a problem hiding this comment.
Re-reviewed. Scope is clean -- only favicon.svg and the link rel icon in index.html. Fixed the color from LinkedIn blue to GitHub dark (#24292e) as requested. LGTM.
3 participants
Closes #14