PictoPy Landing Page improvements #724#769
PictoPy Landing Page improvements #724#769ritigya03 wants to merge 4 commits intoAOSSIE-Org:mainfrom
Conversation
github-actions
bot
added
UI
documentation
WalkthroughThis pull request updates OpenAPI schema definitions, modifies onboarding step components to remove auto-completion logic and adjust progress indexing from one-based to zero-based, and enhances the landing page with navigation features and updated metadata. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
frontend/src/components/OnboardingSteps/FolderSetupStep.tsx (1)
58-58: Critical: Zero-based step indexing creates confusing UX.Same issue as ThemeSelectionStep.tsx - the zero-based indexing displays "Step 0 of 3" with 0% progress on the first step, which is not user-friendly.
Apply the same fix as ThemeSelectionStep.tsx:
- const progressPercent = Math.round((stepIndex / totalSteps) * 100); + const progressPercent = Math.round(((stepIndex + 1) / totalSteps) * 100);<span> - Step {stepIndex} of {totalSteps} + Step {stepIndex + 1} of {totalSteps} </span>Also applies to: 66-66
frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx (1)
58-58: Critical: Zero-based step indexing creates confusing UX.Same issue affects this file - zero-based indexing results in "Step 0 of 3" with 0% progress for the first step. This violates standard UX conventions.
Apply this diff to restore 1-based display:
<span> - Step {stepIndex} of {totalSteps} + Step {stepIndex + 1} of {totalSteps} </span> - <span>{Math.round((stepIndex / totalSteps) * 100)}%</span> + <span>{Math.round(((stepIndex + 1) / totalSteps) * 100)}%</span><div className="bg-primary h-full rounded-full transition-all duration-300" - style={{ width: `${(stepIndex / totalSteps) * 100}%` }} + style={{ width: `${((stepIndex + 1) / totalSteps) * 100}%` }} />Also applies to: 60-60, 65-65
🧹 Nitpick comments (3)
landing-page/src/Pages/pictopy-landing.tsx (2)
77-87: Consider the onClick notification timing.The download anchors use both
href(for the download) andonClick(for the notification). When a user clicks, the browser may immediately initiate the download via thehref, potentially preventing the notification from being visible for the full 3 seconds. This is a minor UX consideration.The implementation should work as-is, but if the notification doesn't appear reliably, consider using
e.preventDefault()in the onClick handler and programmatically triggering the download after showing the notification, or simply remove the notification if the download starts immediately.Also applies to: 90-100, 103-113
2-2: Remove the unused Button import.The
Buttoncomponent is imported but no longer used after changing the download elements to anchor tags.Apply this diff:
-import { Button } from "@/components/ui/button";frontend/src/components/OnboardingSteps/OnboardingStep.tsx (1)
13-13: Refactor: Remove unused stepName prop or clarify its purpose.The
stepNameprop is accepted but completely ignored in favor of the derivedcurrentStepName. This creates confusion about which prop controls rendering.Consider either:
- Removing the
stepNameprop entirely if it's not needed- Using it for validation while still deriving the current step from
stepIndexIf removing:
interface OnboardingStepProps { stepIndex: number; - stepName: string; }export const OnboardingStep: React.FC<OnboardingStepProps> = ({ stepIndex, - stepName, // still accepted, but not trusted }) => {Also applies to: 24-24
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
frontend/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (8)
docs/backend/backend_python/openapi.json(1 hunks)frontend/src/components/OnboardingSteps/AvatarSelectionStep.tsx(2 hunks)frontend/src/components/OnboardingSteps/FolderSetupStep.tsx(3 hunks)frontend/src/components/OnboardingSteps/OnboardingStep.tsx(2 hunks)frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx(5 hunks)landing-page/index.html(1 hunks)landing-page/src/Pages/Landing page/Home1.tsx(1 hunks)landing-page/src/Pages/pictopy-landing.tsx(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (2)
frontend/src/features/onboardingSlice.ts (2)
markCompleted(31-42)previousStep(43-50)frontend/src/constants/routes.ts (1)
ROUTES(1-12)
🔇 Additional comments (10)
docs/backend/backend_python/openapi.json (3)
1-2927: Clarify PR scope: API schema changes not mentioned in PR objectives.The PR description focuses exclusively on landing page improvements (smooth scrolling, documentation links, title/branding updates, download buttons). However, this OpenAPI schema file contains backend API changes to the Face Search endpoint and ImageInCluster metadata structure.
Please clarify:
- Is this file change intentional and related to the landing page PR?
- Should this be split into a separate PR?
- Are there other unrelated changes bundled in this PR?
1114-1130: Input type parameter schema change: Valid but verbose.The change wraps the
InputTypereference inallOfand addsdescription,default, andtitlemetadata. This is a valid OpenAPI 3.1.0 pattern and improves API documentation by providing better defaults and descriptions for the parameter.However, note that
allOfwith a single reference is somewhat verbose—adding the metadata properties directly at the parameter level (without the wrapper) would be more concise and equally valid. The current approach works but is not idiomatic OpenAPI.Backwards compatibility: This change is fully backwards compatible. Existing API clients will continue to work without modification.
2204-2214: Metadata field cleanup: Removal of redundant property constraint.The change removes
"additionalProperties": truefrom the metadata field inImageInCluster. This is a cosmetic cleanup sinceadditionalProperties: trueis the default in JSON Schema—removing it has no functional impact on the API contract.Backwards compatibility: No impact. The schema behavior remains identical.
landing-page/index.html (1)
7-7: LGTM!The title update appropriately reflects the application branding and provides clear information to users.
landing-page/src/Pages/Landing page/Home1.tsx (2)
36-54: LGTM!The smooth scroll implementation is clean and user-friendly. The change from anchor to button is semantically correct since this triggers an action rather than navigating to a different page.
58-74: Documentation URL is accessible and implementation is correct.The URL
https://aossie-org.github.io/PictoPy/returns a valid 200 status code and is properly accessible. The code correctly opens the documentation in a new tab usingwindow.openwith the"_blank"target, which is appropriate for external resources. No changes needed.landing-page/src/Pages/pictopy-landing.tsx (2)
20-26: LGTM!The constant extraction and id addition support the smooth scroll functionality implemented in Home1.tsx. Good practice to centralize the base URL.
77-113: Asset filenames are correctly matched to the current release.The download URLs have been verified against the latest GitHub release (v1.0.0). All three asset names match exactly:
- Mac:
PictoPy_aarch64.app.tar.gz✓- Windows:
PictoPy_1.0.0_x64-setup.exe✓- Linux:
PictoPy_1.0.0_amd64.deb✓The apparent version inconsistency (Mac lacks version in filename while Windows/Linux include it) reflects the actual release artifact naming convention, not a code issue. No changes needed.
frontend/src/components/OnboardingSteps/ThemeSelectionStep.tsx (2)
1-7: Navigation setup looks good.The React Router integration is correctly implemented for navigating to the home page after theme selection.
Also applies to: 36-36
42-46: No action needed. ThemeSelectionStep is intentionally the final visible step in the onboarding flow (step 3 of 3 in VISIBLE_STEPS), making navigation to HOME the correct behavior. The UPDATE_STEP exists in the codebase but is not included in the user-facing onboarding sequence.
|
|
||
| {/* Windows */} | ||
| <a | ||
| href={`${GITHUB_LATEST_RELEASE}/PictoPy_1.0.0_x64-setup.exe`} |
There was a problem hiding this comment.
Hardcoding a link in this way creates a problem. Let us say that when we release version 1.1.0, the file name will change, and the link will no longer work.
|
Sorry, I have to close this PR because of the problem I mentioned above. |
2 participants
This PR addresses the requirements mentioned in Issue #724 by implementing only the requested changes.
The changes are intentionally kept minimal and focused, preserving the existing landing page design and behavior without introducing additional logic or features.
Team - EtherX
PR LINK - #769