-
Notifications
You must be signed in to change notification settings - Fork 0
Apply review suggestions #55
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR addresses review feedback from PR #6059, focusing on improving the scrollbar styling to match the desktop app's JASPScrollBar component, adding error logging for Qt WebChannel operations, and implementing asset fallback logic.
Key changes:
- Added error logging to Qt WebChannel initialization and info fetching
- Implemented custom scrollbar component with padded borders matching desktop styling
- Added fallback logic to find compatible assets when pre-release assets are unavailable
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/useJaspQtObject.ts | Added console.error logging for Qt WebChannel errors |
| src/styles.css | Added jasp-gray-darker color variable and removed scrollbar styles from body |
| src/App.tsx | Added JaspScrollbar component and asset fallback logic in getReleaseInfo |
| public/index.json | Updated module download counts and release URLs (data update) |
| package.json | Version bump from 0.4.1 to 0.4.2 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| <div className="border-jasp-gray-darker border-r-1"> | ||
| <div className="scrollbar-thin scrollbar-thumb-ring scrollbar-hover:scrollbar-thumb-thumb-hover scrollbar-track-popover h-screen overflow-y-auto border-popover border-r-1"> | ||
| <div className="min-h-screen border-popover border-r-1"> | ||
| <div className="min-h-screen border-jasp-gray-darker border-r-1"> | ||
| {children} | ||
| </div> | ||
| </div> | ||
| </div> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
Copilot
AI
Nov 14, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The JaspScrollbar component has 4 nested divs which creates unnecessary DOM nesting and makes it difficult to maintain. Consider consolidating the borders into fewer wrapper divs or using a single div with compound border styles if the visual effect allows it.
| <div className="border-jasp-gray-darker border-r-1"> | |
| <div className="scrollbar-thin scrollbar-thumb-ring scrollbar-hover:scrollbar-thumb-thumb-hover scrollbar-track-popover h-screen overflow-y-auto border-popover border-r-1"> | |
| <div className="min-h-screen border-popover border-r-1"> | |
| <div className="min-h-screen border-jasp-gray-darker border-r-1"> | |
| {children} | |
| </div> | |
| </div> | |
| </div> | |
| </div> | |
| ); | |
| } | |
| <div | |
| className={cn( | |
| // Combine all border and scrollbar classes from the nested divs | |
| "border-jasp-gray-darker border-popover border-r-1 scrollbar-thin scrollbar-thumb-ring scrollbar-hover:scrollbar-thumb-thumb-hover scrollbar-track-popover h-screen min-h-screen overflow-y-auto" | |
| )} | |
| > | |
| {children} | |
| </div> | |
| ); | |
| } |
Some feedback was given in jasp-stats/jasp-desktop#6059 which needs to be resolved here.
TODO
NICE to haves