feat: add screenshots to the landing page#588
Draft
Aeledfyr wants to merge 2 commits intoNerimity:mainfrom
Draft
feat: add screenshots to the landing page#588Aeledfyr wants to merge 2 commits intoNerimity:mainfrom
Aeledfyr wants to merge 2 commits intoNerimity:mainfrom
Conversation
Aeledfyr
force-pushed
the
landing-screenshots
branch
from
0f7f960 to
9939468
Compare
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a responsive screenshot gallery layout to HomePage with CSS styling for a three-panel image display and mobile responsiveness, along with a new internal ScreenshotView component to render the images. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/pages/HomePage.module.css (2)
160-167: Redundantbottom: unsetdeclaration.Line 162 sets
bottom: unsetbut Line 163 immediately overwrites it withbottom: 8%. Theunsetline appears to be a copy-paste artifact from.screenshotCenterand can be removed.🧹 Proposed fix
.screenshotLeft { position: absolute; - bottom: unset; bottom: 8%; left: 7%; width: 85%; z-index: 2; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/HomePage.module.css` around lines 160 - 167, Remove the redundant bottom: unset declaration inside the .screenshotLeft CSS rule (it is immediately overwritten by bottom: 8%); update the .screenshotLeft block by deleting the bottom: unset line so only bottom: 8% remains, leaving other properties (position, left, width, z-index) unchanged.
53-53: Stylelint formatting issues.Static analysis flags empty line inconsistencies:
- Line 53: Unexpected empty line before
margin-topdeclaration- Line 102: Missing empty line before
displaydeclarationThese can be auto-fixed with
stylelint --fixif enforced in CI.Also applies to: 102-102
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/pages/HomePage.module.css` at line 53, Fix the Stylelint formatting issues by removing the unexpected empty line before the margin-top declaration and adding the missing empty line before the display declaration (i.e., adjust the spacing around the `margin-top` and `display` declarations to match the project's CSS style rules), or simply run `stylelint --fix` to auto-correct the file; ensure the `margin-top` and `display` declarations now follow the same empty-line conventions as other rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/pages/HomePage.tsx`:
- Around line 283-289: The three <img> elements in the ScreenshotView component
lack alt attributes; add accessible, translatable alt text by using the
translation context: call useTransContext() at the top of ScreenshotView (e.g.,
const [t] = useTransContext();) and pass descriptive keys to t for each
screenshot (e.g., t('screenshot.centerAlt'), t('screenshot.leftAlt'),
t('screenshot.rightAlt')) and wire those returned strings into the alt attribute
of each <img> element; ensure the same pattern is applied to the other two image
groups referenced in the comment.
---
Nitpick comments:
In `@src/pages/HomePage.module.css`:
- Around line 160-167: Remove the redundant bottom: unset declaration inside the
.screenshotLeft CSS rule (it is immediately overwritten by bottom: 8%); update
the .screenshotLeft block by deleting the bottom: unset line so only bottom: 8%
remains, leaving other properties (position, left, width, z-index) unchanged.
- Line 53: Fix the Stylelint formatting issues by removing the unexpected empty
line before the margin-top declaration and adding the missing empty line before
the display declaration (i.e., adjust the spacing around the `margin-top` and
`display` declarations to match the project's CSS style rules), or simply run
`stylelint --fix` to auto-correct the file; ensure the `margin-top` and
`display` declarations now follow the same empty-line conventions as other
rules.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
public/assets/screenshot-center.pngis excluded by!**/*.pngpublic/assets/screenshot-left.pngis excluded by!**/*.pngpublic/assets/screenshot-right.pngis excluded by!**/*.pngpublic/assets/screenshots/center-2400w-base.pngis excluded by!**/*.pngpublic/assets/screenshots/left-1720w-base.pngis excluded by!**/*.pngpublic/assets/screenshots/left-860w-fallback.jpgis excluded by!**/*.jpgpublic/assets/screenshots/right-1170w-base.pngis excluded by!**/*.pngpublic/assets/screenshots/right-585w-fallback.jpgis excluded by!**/*.jpg
📒 Files selected for processing (20)
public/assets/screenshots/center-1200w.jxlpublic/assets/screenshots/center-1200w.webppublic/assets/screenshots/center-2400w.jxlpublic/assets/screenshots/center-2400w.webppublic/assets/screenshots/center-600w.jxlpublic/assets/screenshots/center-600w.webppublic/assets/screenshots/left-1720w.jxlpublic/assets/screenshots/left-1720w.webppublic/assets/screenshots/left-430w.jxlpublic/assets/screenshots/left-430w.webppublic/assets/screenshots/left-860w.jxlpublic/assets/screenshots/left-860w.webppublic/assets/screenshots/right-1170w.jxlpublic/assets/screenshots/right-1170w.webppublic/assets/screenshots/right-293w.jxlpublic/assets/screenshots/right-293w.webppublic/assets/screenshots/right-585w.jxlpublic/assets/screenshots/right-585w.webpsrc/pages/HomePage.module.csssrc/pages/HomePage.tsx
Aeledfyr
force-pushed
the
landing-screenshots
branch
from
9939468 to
06a6769
Compare
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
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.
This adds some screenshots to the landing page, to give people a better idea of what Nerimity actually looks like, and hopefully to convince them to try it.
The images are available in multiple sizes and selected with srcset. There are jpegxl images and webp images for each size, and then a fallback jpeg at the median size for each. (The .jxl images have slightly higher quality than the .webp images, at least for color preservation, and are used when supported.)
This ends up adding about 500 KiB to the page load for desktop and about 400 KiB for mobile. The current page is about 360 KiB, so it roughly doubles the page size, but the images are set to lazy load, so it shouldn't slow down the page much. There are matching colored backgrounds behind each image, so the slow image loading should be relatively unobtrusive.
There are a few current concerns with this:
Screenshots
Desktop above-the-fold view:
Desktop scrolled view:
Mobile above-the-fold view:
Mobile scrolled view:
Did you test your code?
Tested on Firefox & Chrome on desktop and Chrome on Android; tested with varying network speeds in both Firefox and Chrome devtools, and with Lighthouse in Chrome devtools.
Checklist
Summary by CodeRabbit
New Features
Styling