Conversation
|
Unable to perform a code review. You have run out of credits 😔 |
Reviewer's Guide by SourceryThis pull request modifies the MarketSelectorButton component to improve the display of market information and ensure responsiveness across different screen sizes. It introduces a mobile-specific layout and enhances the desktop version's styling. No diagrams generated as the changes look simple and do not need a visual representation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Manifest Files |
Deploying champion-trader with
|
| Latest commit: |
63c4b4a
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://ff036145.champion-trader.pages.dev |
| Branch Preview URL: | https://hot-fix.champion-trader.pages.dev |
There was a problem hiding this comment.
Hey @aum-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- It looks like you're duplicating a lot of code between the mobile and non-mobile versions - can you extract the common parts into a separate component or function?
Here's what I looked at during the review
- 🟡 General issues: 1 issue found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| return ( | ||
| <div className="h-14 inline-flex cursor-pointer lg:mt-3" data-id="market-info"> | ||
| <div | ||
| className="h14 flex items-center gap-4 px-4 bg-theme-secondary rounded-lg font-medium" |
There was a problem hiding this comment.
issue (typo): Potential typo in height class name in the mobile branch.
The class name is written as 'h14' instead of 'h-14', which might be a typo. Correcting this would ensure consistent sizing as intended.
| className="h14 flex items-center gap-4 px-4 bg-theme-secondary rounded-lg font-medium" | |
| className="h-14 flex items-center gap-4 px-4 bg-theme-secondary rounded-lg font-medium" |
| }, [price, subtitle]); | ||
|
|
||
| if (isMobile) { | ||
| return ( |
There was a problem hiding this comment.
issue (complexity): Consider creating a new MarketInfoContent component to avoid duplicating the render logic for the icon, title, closed badge, and subtitle in both mobile and non-mobile views, improving maintainability and readability without changing functionality
You can reduce duplication by extracting the shared UI into a separate, stateless component. For example, create a new component that renders the icon, title, closed badge, and subtitle. Then call it from both the mobile and non-mobile branches. For instance:
// New reusable component
const MarketInfoContent = ({
instrument,
symbol,
isLoading,
displayName,
isClosed,
displaySubtitle,
iconClass = "w-8 h-8 flex items-center justify-center",
titleClass = "text-sm font-semibold text-theme leading-6 truncate",
chevronClass = "w-5 text-theme flex-shrink-0 stroke-[1.5]",
}) => (
<>
{instrument ? (
<div className={iconClass}>
<MarketIcon symbol={symbol} size="xlarge" showBadge={false} />
</div>
) : (
<div className={iconClass}>
{isLoading && (
<div className="w-6 h-6 border-2 border-t-transparent border-theme rounded-full animate-spin"></div>
)}
</div>
)}
<div className="min-w-0">
<div className="flex items-center gap-2">
<div className={titleClass}>{displayName}</div>
{isClosed && (
<span className="text-[10px] px-1.5 py-0.5 bg-rose-500/10 rounded-full text-rose-500">
Closed
</span>
)}
<ChevronDown className={chevronClass} />
</div>
<div className="text-[12px] text-theme leading-5 truncate">{displaySubtitle}</div>
</div>
</>
);Then update the original render logic to use the new component:
if (isMobile) {
return (
<div className="h-14 inline-flex cursor-pointer lg:mt-3" data-id="market-info">
<div className="h14 flex items-center gap-4 px-4 bg-theme-secondary rounded-lg font-medium" onClick={handleClick}>
<MarketInfoContent
instrument={instrument}
symbol={symbol}
isLoading={isLoading}
displayName={displayName}
isClosed={isClosed}
displaySubtitle={displaySubtitle}
// Optionally override icon/title/chevron classes if needed for mobile
/>
</div>
</div>
);
}
return (
<div
className="h-14 inline-flex cursor-pointer bg-theme-secondary hover:bg-theme-hover active:bg-theme-active rounded-lg transition-colors"
data-id="market-info"
onClick={handleClick}
>
<div className="h-14 flex items-center gap-4 px-4">
<MarketInfoContent
instrument={instrument}
symbol={symbol}
isLoading={isLoading}
displayName={displayName}
isClosed={isClosed}
displaySubtitle={displaySubtitle}
/>
</div>
</div>
);By extracting the common UI into MarketInfoContent, you remove duplicate render logic, making the component easier to maintain and read without changing any behavior.
Summary by Sourcery
Enhancements: