-
-
Notifications
You must be signed in to change notification settings - Fork 42
FEAT: Added tabs to easily navigate between description and solution #177
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
base: main
Are you sure you want to change the base?
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 introduces a tabbed interface allowing users to switch between the lesson description and the solution code.
- Added
TabType
and integratedselectedTab
into shared types and store. - Created
Tabs
,TabHeader
, andSolutionTab
components for the new UI. - Refactored
EditorNOutput
to use globalcodeString
state and initialized it on mount.
Reviewed Changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
File | Description |
---|---|
lib/types.ts | Defined TabType and added selectedTab to OutputResult . |
lib/stores.ts | Extended user store with codeString and setCodeString setter. |
app/content/[...markdownPath]/page.tsx | Wrapped content in new Tabs component instead of ContentViewer . |
app/content/[...markdownPath]/page.module.css | Styled new .tabContainer to position the tabs. |
app/components/Tabs/index.tsx | Re-exported Tabs component. |
app/components/Tabs/Tabs.tsx | Implemented tab logic and content switching. |
app/components/Tabs/Tabs.module.css | Base styles for the Tabs container. |
app/components/TabHeader/index.tsx | Re-exported TabHeader component. |
app/components/TabHeader/TabHeader.tsx | Rendered tab buttons and underline indicator. |
app/components/TabHeader/TabHeader.module.css | Styles for tab header layout and active state. |
app/components/SolutionTab/index.tsx | Re-exported SolutionTab component. |
app/components/SolutionTab/SolutionTab.tsx | Embedded read-only Monaco editor for solutions and “Use this” btn. |
app/components/SolutionTab/SolutionTab.module.css | Styles for SolutionTab container. |
app/components/MyBtn/MyBtn.tsx | Added position prop to float the button. |
app/components/EditorNOutput/EditorNOutput.tsx | Switched to global codeString state and set initial code. |
app/components/ContentViewer/ContentViewer.module.css | Added top margin to account for fixed tab header. |
Comments suppressed due to low confidence (3)
app/components/SolutionTab/SolutionTab.tsx:36
- Rename
useSolution
to something likehandleUseSolution
since it is not a React hook and avoids confusion with custom hook conventions.
function useSolution() {
app/components/MyBtn/MyBtn.tsx:11
- [nitpick] The prop name
position
may be ambiguous; consider renaming toalign
orfloatDirection
to better convey its purpose.
position = "right",
app/components/Tabs/Tabs.tsx:1
- There are no tests covering the new tab-switching behavior; consider adding unit or integration tests for
Tabs
andTabHeader
interactions.
"use client";
|
||
<ContentViewer> | ||
{ | ||
currentSelectedTab=="description"? |
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.
Use strict equality (===
) instead of loose equality (==
) for string comparisons to avoid unintended type coercion.
currentSelectedTab=="description"? | |
currentSelectedTab==="description"? |
Copilot uses AI. Check for mistakes.
}: TabHeaderProps & { | ||
currentSelectedTab: "description" | "solution"; | ||
setCurrentSelectedTab: (tab: "description" | "solution") => void; | ||
}) => { |
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 inline intersection with TabHeaderProps
is redundant; use TabHeaderProps
alone for clearer, DRY type annotations.
}: TabHeaderProps & { | |
currentSelectedTab: "description" | "solution"; | |
setCurrentSelectedTab: (tab: "description" | "solution") => void; | |
}) => { | |
}: TabHeaderProps) => { |
Copilot uses AI. Check for mistakes.
@@ -0,0 +1 @@ | |||
export { default as default } from "./Tabs"; |
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.
[nitpick] Simplify the re-export to export { default } from "./Tabs";
for clearer syntax.
export { default as default } from "./Tabs"; | |
export { default } from "./Tabs"; |
Copilot uses AI. Check for mistakes.
<div className={styles.tabContainer}> | ||
<button | ||
onClick={() => setCurrentSelectedTab("description")} | ||
className={`${styles.tabButton} ${ | ||
currentSelectedTab === "description" ? styles.activeTab : "" | ||
}`} |
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.
Consider adding ARIA roles (e.g., role="tablist"
on the container and role="tab"
with aria-selected
on buttons) to improve keyboard navigation and screen reader support.
<div className={styles.tabContainer}> | |
<button | |
onClick={() => setCurrentSelectedTab("description")} | |
className={`${styles.tabButton} ${ | |
currentSelectedTab === "description" ? styles.activeTab : "" | |
}`} | |
<div className={styles.tabContainer} role="tablist"> | |
<button | |
onClick={() => setCurrentSelectedTab("description")} | |
className={`${styles.tabButton} ${ | |
currentSelectedTab === "description" ? styles.activeTab : "" | |
}`} | |
role="tab" | |
aria-selected={currentSelectedTab === "description"} |
Copilot uses AI. Check for mistakes.
@@ -64,6 +63,9 @@ export default function EditorNOutput({ | |||
}; | |||
|
|||
useEffect(() => { | |||
// Load the initial code | |||
setCodeString(JSON.stringify(codeFile.code, null, 2)); |
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.
Include codeFile
in the useEffect
dependency array to ensure the editor state resets if the content changes.
Copilot uses AI. Check for mistakes.
@@ -31,6 +33,7 @@ export default function MyBtn({ | |||
textTransform={"uppercase"} | |||
isDisabled={isDisabled} | |||
fontWeight={"bold"} | |||
style={{float: position}} |
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.
Using inline float
styling can lead to layout issues; consider leveraging Chakra UI props (e.g., alignSelf
) or a container layout instead.
Copilot uses AI. Check for mistakes.
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.
@AQIB-NAWAB I see we have modified around 17 files to implement this feature. Can we improve this and make changes to only the relevant files? if we need to change these many files, please add explanations of the changes (as code review comments) in the files on why we need this change and how it is relevant to the feature.
Moreover, we are using the text editor component to display the solution. Can we use the markdown code block instead?
const { colorMode } = useColorMode(); | ||
const [monaco, setMonaco] = useState<any>(null); | ||
const [isValidating, setIsValidating] = useState(false); | ||
const editorStore = useEditorStore(); | ||
const editorRef = useRef<any>(null); | ||
|
||
// Apply custom hooks | ||
useEditorTheme(monaco, colorMode); |
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.
Please remove these formatting chagnes
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.
Can we just use the chakraUI tabs instead (of course with customized styles)
What kind of change does this PR introduce?
Add the tab feature so user can easily navigate between description and solution. This feature enhance user experience.
Issue Number:
Screenshots/videos:
Screencast from 2025-06-08 11-29-15.webm
Summary
Allow users to easily compare there code and improve there user experience.
Does this PR introduce a breaking change?
No