551: Fix Leaderboard Card Ordering On Mobile View#902
551: Fix Leaderboard Card Ordering On Mobile View#902isabellalam12 wants to merge 2 commits intomainfrom
Conversation
Available PR Commands
See: https://github.com/tahminator/codebloom/wiki/CI-Commands |
Title551: Fix Leaderboard Card Ordering On Mobile View PR TypeBug fix Description
Diagram Walkthroughflowchart LR
A[Dashboard Page] --> B{Is `smallPhone`?};
B -- Yes --> C[Problem of the Day then Leaderboard];
B -- No --> D[Leaderboard then Problem of the Day];
C --> E[Render Recent Submissions];
D --> E;
|
| Relevant files | |||||
|---|---|---|---|---|---|
| Enhancement |
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| <Flex direction={smallPhone ? "row" : "column"} gap={"md"}> | ||
| <Flex direction={"column"} flex={1}> | ||
| <ProblemOfTheDay /> | ||
| </Flex> | ||
| <Flex direction={"column"} flex={1}> | ||
| <DashboardLeaderboard | ||
| userId={data.user.id} | ||
| userTags={data.user.tags} | ||
| /> | ||
| </Flex> | ||
| {smallPhone ? | ||
| <> | ||
| <Flex direction={"column"} flex={1}> | ||
| <ProblemOfTheDay /> | ||
| </Flex> | ||
| <Flex direction={"column"} flex={1}> | ||
| <DashboardLeaderboard | ||
| userId={data.user.id} | ||
| userTags={data.user.tags} | ||
| /> | ||
| </Flex> | ||
| </> | ||
| : <> | ||
| <Flex direction={"column"} flex={1}> | ||
| <DashboardLeaderboard | ||
| userId={data.user.id} | ||
| userTags={data.user.tags} | ||
| /> | ||
| </Flex> | ||
| <Flex direction={"column"} flex={1}> | ||
| <ProblemOfTheDay /> | ||
| </Flex> | ||
| </> | ||
| } | ||
| <Flex direction={"column"} flex={1}> | ||
| <RecentSubmissions userId={data.user.id} /> | ||
| </Flex> |
There was a problem hiding this comment.
Suggestion: The direction prop of the main Flex component appears to be inverted for mobile and desktop views. For smallPhone (mobile), the Flex direction should typically be column to stack content vertically, while for larger screens, it could be row for a horizontal layout. The current implementation sets direction="row" for smallPhone and direction="column" otherwise, which is likely to cause layout issues on mobile. [possible issue, importance: 8]
New proposed code:
-<Flex direction={smallPhone ? "row" : "column"} gap={"md"}>
+<Flex direction={smallPhone ? "column" : "row"} gap={"md"}>
{smallPhone ?
<>
<Flex direction={"column"} flex={1}>
<ProblemOfTheDay />
</Flex>
<Flex direction={"column"} flex={1}>
<DashboardLeaderboard
userId={data.user.id}
userTags={data.user.tags}
/>
</Flex>
</>
: <>
<Flex direction={"column"} flex={1}>
<DashboardLeaderboard
userId={data.user.id}
userTags={data.user.tags}
/>
</Flex>
<Flex direction={"column"} flex={1}>
<ProblemOfTheDay />
</Flex>
</>
}
<Flex direction={"column"} flex={1}>
<RecentSubmissions userId={data.user.id} />
</Flex>
</Flex>| <CodebloomCard | ||
| miw={{ base: "31vw", md: "31vw" }} | ||
| mih={{ base: "auto", md: "63vh" }} | ||
| > |
There was a problem hiding this comment.
Suggestion: The miw prop for CodebloomCard uses responsive syntax but assigns the same value to both base and md breakpoints. This makes the md breakpoint specification redundant. Simplify this by either removing the md entry or by directly using miw="31vw" if the width is intended to be consistent across these breakpoints. [general, importance: 5]
| <CodebloomCard | |
| miw={{ base: "31vw", md: "31vw" }} | |
| mih={{ base: "auto", md: "63vh" }} | |
| > | |
| <CodebloomCard | |
| miw="31vw" | |
| mih={{ base: "auto", md: "63vh" }} | |
| > |
Title551: Fix Leaderboard Card Ordering On Mobile View PR TypeEnhancement Description
Diagram Walkthroughflowchart LR
A[Dashboard Page] --> B{Is smallPhone?};
B -- Yes --> C[ProblemOfTheDay then DashboardLeaderboard];
B -- No --> D[DashboardLeaderboard then ProblemOfTheDay];
|
| Relevant files | |||||
|---|---|---|---|---|---|
| Enhancement |
|
PR Reviewer Guide 🔍Here are some key observations to aid the review process:
|
| {smallPhone ? | ||
| <> | ||
| <Flex direction={"column"} flex={1}> | ||
| <ProblemOfTheDay /> | ||
| </Flex> | ||
| <Flex direction={"column"} flex={1}> | ||
| <DashboardLeaderboard | ||
| userId={data.user.id} | ||
| userTags={data.user.tags} | ||
| /> | ||
| </Flex> | ||
| </> | ||
| : <> | ||
| <Flex direction={"column"} flex={1}> | ||
| <DashboardLeaderboard | ||
| userId={data.user.id} | ||
| userTags={data.user.tags} | ||
| /> | ||
| </Flex> | ||
| <Flex direction={"column"} flex={1}> | ||
| <ProblemOfTheDay /> | ||
| </Flex> | ||
| </> | ||
| } |
There was a problem hiding this comment.
Suggestion: The conditional rendering of ProblemOfTheDay and DashboardLeaderboard components introduces significant code duplication. This can be refactored by defining the components in an array and conditionally ordering them based on smallPhone before mapping and rendering them. This approach enhances readability and maintainability. [general, importance: 8]
| {smallPhone ? | |
| <> | |
| <Flex direction={"column"} flex={1}> | |
| <ProblemOfTheDay /> | |
| </Flex> | |
| <Flex direction={"column"} flex={1}> | |
| <DashboardLeaderboard | |
| userId={data.user.id} | |
| userTags={data.user.tags} | |
| /> | |
| </Flex> | |
| </> | |
| : <> | |
| <Flex direction={"column"} flex={1}> | |
| <DashboardLeaderboard | |
| userId={data.user.id} | |
| userTags={data.user.tags} | |
| /> | |
| </Flex> | |
| <Flex direction={"column"} flex={1}> | |
| <ProblemOfTheDay /> | |
| </Flex> | |
| </> | |
| } | |
| {(() => { | |
| const leaderboardCard = ( | |
| <Flex direction={"column"} flex={1}> | |
| <DashboardLeaderboard | |
| userId={data.user.id} | |
| userTags={data.user.tags} | |
| /> | |
| </Flex> | |
| ); | |
| const problemOfTheDayCard = ( | |
| <Flex direction={"column"} flex={1}> | |
| <ProblemOfTheDay /> | |
| </Flex> | |
| ); | |
| const orderedComponents = smallPhone | |
| ? [problemOfTheDayCard, leaderboardCard] | |
| : [leaderboardCard, problemOfTheDayCard]; | |
| return <>{orderedComponents}</>; | |
| })()} |
1 participant
551
Description of changes
Checklist before review
Screenshots
Dev
Staging