Fixing Duration Style for Desktop View #36
Conversation
Deploying champion-trader with
|
| Latest commit: |
0f15d65
|
| Status: | 🚫 Build failed. |
Reviewer's Guide by SourceryThis pull request focuses on improving the styling and layout of the duration selection component, especially for desktop view. It introduces the use of DesktopTradeFieldCard to provide a consistent look and feel across the trade form. Additionally, it updates the styles of the TabList component and DesktopTradeFieldCard, and adds a background color to the MarketInfo component. 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 |
ahmadtaimoor-deriv
changed the title
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @ahmadtaimoor-deriv - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider adding a visual cue to indicate when the
DurationFieldis selected on desktop, similar to theStakeField. - The change to
VerticalTabListintroduces a hardcoded color; consider using a theme variable or Tailwind class for better maintainability.
Here's what I looked at during the review
- 🟢 General issues: all looks good
- 🟢 Security: all looks good
- 🟡 Testing: 2 issues found
- 🟡 Complexity: 2 issues 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.
| @@ -20,7 +20,7 @@ describe('DesktopTradeFieldCard', () => { | |||
| ); | |||
|
|
|||
| const card = container.firstChild as HTMLElement; | |||
| expect(card).toHaveClass('bg-white', 'rounded-lg', 'p-2'); | |||
| expect(card).toHaveClass('rounded-lg', 'p-2','bg-[rgba(246,247,248,1)]','border','border-transparent'); | |||
There was a problem hiding this comment.
suggestion (testing): Test the isSelected style
Add a test case to verify the styles when the isSelected prop is true. This ensures the visual feedback for the selected state is correctly applied.
Suggested implementation:
it('merges custom className with default styles', () => {
// existing test code...
});
// New test to verify styles when isSelected is true it('applies isSelected style when isSelected prop is true', () => {
// Render the component with the isSelected prop set to true.
const { container } = render(<DesktopTradeFieldCard isSelected={true} />);
const card = container.firstChild as HTMLElement;
// Expect the card to have default styles plus the selected border style.
expect(card).toHaveClass('rounded-lg', 'p-2', 'bg-[rgba(246,247,248,1)]', 'border', 'border-blue-500');
});Make sure that the component DesktopTradeFieldCard is imported in this test file if it isn't already. For example:
<<<<<<< SEARCH
// (No import of DesktopTradeFieldCard)
import DesktopTradeFieldCard from '../DesktopTradeFieldCard';
REPLACE
Also ensure that test utilities like render are imported from the React Testing Library.
| @@ -31,6 +31,6 @@ describe('DesktopTradeFieldCard', () => { | |||
| ); | |||
|
|
|||
| const card = container.firstChild as HTMLElement; | |||
| expect(card).toHaveClass('bg-white p-2 rounded-lg'); | |||
| expect(card).toHaveClass('bg-[rgba(246,247,248,1)] rounded-lg p-2 border border-transparent custom-class'); | |||
There was a problem hiding this comment.
suggestion (testing): Test the error style
It's important to also test the error styles. Add a test case where the error prop is true to ensure the visual feedback for errors is correctly displayed.
Suggested implementation:
it('applies error styles when error prop is true', () => {
const { container } = render(<DesktopTradeFieldCard error />);
const card = container.firstChild as HTMLElement;
expect(card).toHaveClass('bg-[rgba(246,247,248,1)]', 'rounded-lg', 'p-2', 'border', 'border-red-500');
});
});Make sure that the DesktopTradeFieldCard component applies "border-red-500" (or the correct error class for your design) when the error prop is true. Adjust expected class names in the test if your design uses different classes.
|
|
||
| interface TradeFormControllerProps { | ||
| isLandscape: boolean; |
There was a problem hiding this comment.
issue (complexity): Consider extracting the repeated reduce logic and SSE event handlers into helper functions.
Consider extracting the repeated reduce logic and SSE event handlers into helper functions to reduce inline nesting. For example, you could create a helper to compute the payout mapping:
// helpers.ts
export const computePayoutValues = (
buttonStates: Record<string, ButtonState>,
targetAction: string,
newPayout: number
): Record<string, number> =>
Object.keys(buttonStates).reduce((acc, key) => {
acc[key] = key === targetAction ? newPayout : buttonStates[key]?.payout || 0;
return acc;
}, {} as Record<string, number>);Then in your component:
import { computePayoutValues } from "./helpers";
// Inside onMessage callback:
onMessage: (priceData) => {
const payoutValue = Number(priceData.price);
setButtonStates((prev) => ({
...prev,
[button.actionName]: {
loading: false,
error: null,
payout: payoutValue,
reconnecting: false,
},
}));
setPayouts({
max: 50000,
values: computePayoutValues(buttonStates, button.actionName, payoutValue),
});
},Similarly, consider moving repeated SSE handler logic into a helper or a custom hook to further reduce inline nesting while keeping functionality intact.
| className={className} | ||
| /> | ||
|
|
||
| {isDesktop ? ( |
There was a problem hiding this comment.
issue (complexity): Consider extracting the conditional wrapper of the "TradeParam" component to reduce JSX duplication.
Consider extracting the conditional wrapper into its own component or helper function so that you only render the <TradeParam> once. This reduces the duplicated props and branching in your JSX.
For example, you can create a simple wrapper component:
const ConditionalWrapper: React.FC<{ children: React.ReactNode }> = ({ children }) =>
isDesktop ? <DesktopTradeFieldCard isSelected={isOpen}>{children}</DesktopTradeFieldCard> : <>{children}</>;
return (
<div className="relative">
<ConditionalWrapper>
<TradeParam
label="Duration"
value={duration}
onClick={handleClick}
className={className}
/>
</ConditionalWrapper>
{isDesktop && isOpen && (
<Popover
isOpen={isOpen}
onClose={handleClose}
style={{
position: "absolute",
right: "100%",
top: "-8px",
marginRight: "16px",
}}
>
<DurationController onClose={handleClose} />
</Popover>
)}
</div>
);This approach retains all functionality while reducing duplication and simplifying the rendering logic.
Summary by Sourcery
This pull request refactors the Duration field component to use the new
DesktopTradeFieldCardcomponent, and improves the UI of the tab list component.Enhancements:
DesktopTradeFieldCardcomponent for a more consistent look and feel.