Download dti logo as png/svg on right click - #1155
Conversation
|
[diff-counting] Significant lines: 281. |
jane-leon
left a comment
jane-leon
left a comment
There was a problem hiding this comment.
hey val!! omg i actually really love this idea, the screenshot problem is so real, having download as svg/png is such a clean solution and feels really polished. I do suggest that next time you start a separate repo , instead of adding on top of alumni. Additionally, I believe you're alumni work has been merged already so do git pull origin main (or rebasing onto main) before you start working/commit again to avoid merge conflicts.
A few things to flag before merge:
-
build is failing since there's a lint error breaking CI: ./src/components/Alumni/Alumni.tsx . I left a comment on how to fix.
-
some other lint/type warnings worth checking. The build also flagged a react-hooks/exhaustive-deps warning once the existing useLayoutEffect in Navbar.tsx:113 (missing navLinks dep). That one's pre-existing but could be cleaned up by moving navLinks outside the component since it's a static array.
I also left some inline comments on specific issues but nothing huge. mostly small polish stuff! Anyways, thank you so much for your work Val!!
| document.addEventListener('touchmove', lockTouch, { passive: false, capture: true }); | ||
| document.addEventListener('wheel', lockWheel, { passive: false, capture: true }); | ||
|
|
||
| return () => { |
There was a problem hiding this comment.
The ESLint error is due to inconsistent returns in the useEffect. The early return (if (!filtersOpen) return;) conflicts with the cleanup function return. To fix this, make the early return explicit by doing
if (!filtersOpen) return undefined; on line 40 of this file.
| // ####################### | ||
|
|
||
| const [contextMenu, setContextMenu] = useState<{ x: number; y: number } | null>(null); | ||
| const logoRef = useRef<HTMLImageElement>(null); |
There was a problem hiding this comment.
LogoRef is unused. Consider deleting it if not using it.
| const [contextMenu, setContextMenu] = useState<{ x: number; y: number } | null>(null); | ||
| const logoRef = useRef<HTMLImageElement>(null); | ||
|
|
||
| const handleLogoContextMenu = (e: React.MouseEvent) => { |
There was a problem hiding this comment.
I noticed that when you right-click near the right or bottom edge of the logo, the menu gets cut off if your browser is small. native browser menus usually auto-flip but this one just clips. could we clamp the position? something like:
const menuWidth = 180;
const menuHeight = 80;
setContextMenu({
x: Math.min(e.clientX, window.innerWidth - menuWidth),
y: Math.min(e.clientY, window.innerHeight - menuHeight),
});
| } | ||
| }; | ||
|
|
||
| useEffect(() => { |
There was a problem hiding this comment.
another small thing i noticed pressing escape doesn't close the menu of the download options. It only closes on click or scroll rn. could we add a keydown listener too?
3 participants
Summary
In the past when DTI did collaborations between organizations, if another organization made slides, they would frequently take screenshots of DTI's logos! A solution I've seen among corporate/startups is that they'll have a download as SVG or even PNG (to preserve quality)!
Test Plan
Run locally, test that the png and svg can both be downloaded and also be accessed when testing!
Notes
Breaking Changes