enhance(apps/docs): improve and extend landing page content and layout #4304
Conversation
Current Aviator status
This PR was merged manually (without Aviator). Merging manually can negatively impact the performance of the queue. Consider using Aviator next time.
See the real-time status of this PR on the
Aviator webapp.
Use the Aviator Chrome Extension
to see the status of your PR within GitHub.
|
|
Caution Review failedThe pull request is closed. 📝 Walkthrough📝 WalkthroughWalkthroughThis pull request introduces several new React functional components within the landing section of the application, including Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
gamification_docs
|
| Run status |
|
| Run duration | 11m 17s |
| Commit |
|
| Committer | Chris |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
Failures
|
0
|
Flaky
|
0
|
Pending
|
0
|
Skipped
|
0
|
Passing
|
140
|
| View all changes introduced in this branch ↗︎ | |
There was a problem hiding this comment.
The hover effects are already very cool :) And a nicer component structure is also always a plus. I just added some comments to smaller styling ideas, which I had - if you disagree or have another idea, we're completely open for that :)
We just need to remember to add corresponding images to all sections before merging it. Also, the discussed counters could definitely be a nice add-on (maybe not too prominently centered, but somewhere further down - to give an impression that the tool is actually used)
| <p className="mt-6 text-lg leading-8 text-gray-600"> | ||
| {description} | ||
| </p> | ||
| <dl className=" mt-10 max-w-xl space-y-8 text-base leading-7 text-gray-600 lg:max-w-none"> |
There was a problem hiding this comment.
Instead of spacing between the elements, would it maybe make sense to use padding on the components themselves? In this way the UI does not switch back to the first element when one changes from the second to the third?
apps/docs/src/pages/index.tsx
Outdated
| description="Interact your students during class and drive engagement with | ||
| your materials." | ||
| imgSrc="/img_v3/sync_interaction.png" | ||
| title="Gamified Learning" |
There was a problem hiding this comment.
Maybe we could make three different blocks like this underneath each other? One for gamification elements, one for synchronous activities and one for asynchronous activities? With somewhat reduced spacing between the text blocks, we could probably even fit it in the same space
There was a problem hiding this comment.
It might be worth checking, whether alternating the size of the image makes it cooler for three successive blocks (like e.g. done on https://www.gbl.uzh.ch/) or whether it just becomes chaotic
| export function CTA() { | ||
| return ( | ||
| <div className="space-y-4 py-16 text-center sm:py-24 md:space-y-8"> | ||
| <H2 className={{ root: 'text-3xl' }}>Be Part of the Journey</H2> |
There was a problem hiding this comment.
As we're restyling the entire landing page, could we maybe also try to improve the styling of these components, make them look more alike the rest and somewhat more interesting? :)
…of the bulletpoints and the first bulletpoint instead the first bulletpoints on to the next page
sjschlapbach
changed the title
todo team section needs to be added
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 7
🧹 Outside diff range and nitpick comments (17)
apps/docs/src/components/landing/UseCaseOverview.tsx (2)
10-31: Consider performance optimization for the grid rendering.The
Object.entries(USE_CASES)creates a new array on each render. Consider memoizing this transformation or moving the USE_CASES entries to a useMemo hook if the component re-renders frequently.+import { useMemo } from 'react' export function UseCaseOverview() { + const useCaseEntries = useMemo(() => Object.entries(USE_CASES), []) return ( <div className="space-y-4"> <H2 className={{ root: 'text-2xl' }}>Use Cases</H2> <div className="grid grid-cols-1 gap-4 rounded md:grid-cols-3"> - {Object.entries(USE_CASES).map(([href, item]) => ( + {useCaseEntries.map(([href, item]) => (
10-10: Consider adding more responsive breakpoints for better layout control.The current grid layout only switches between 1 and 3 columns. For better user experience on tablet-sized devices, consider adding an intermediate 2-column layout.
- <div className="grid grid-cols-1 gap-4 rounded md:grid-cols-3"> + <div className="grid grid-cols-1 gap-4 rounded sm:grid-cols-2 lg:grid-cols-3">apps/docs/src/components/teampage/team.tsx (1)
1-5: Consider enhancing the TeamMember interface with additional fields and validation.The interface could be improved by:
- Adding optional fields for social media links or contact information
- Including image validation to ensure URLs are valid
interface TeamMember { imgSrc: string name: string position: string + socialLinks?: { + linkedin?: string + twitter?: string + github?: string + } + email?: string }apps/docs/src/components/landing/CTA.tsx (3)
3-7: Add accessibility improvements to the component.While the structure is well-organized, consider these accessibility enhancements:
- Add an ARIA label to the container div to describe its purpose
- Consider adding a role="region" to the main container
- <div className="space-y-4 py-16 text-center sm:py-24 md:space-y-8"> + <div + className="space-y-4 py-16 text-center sm:py-24 md:space-y-8" + aria-label="Call to Action Section" + role="region" + >
13-24: Extract common button styles to reduce duplication.Both buttons share identical styling. Consider extracting the common styles to a constant or creating a styled button component.
+ const BUTTON_STYLES = { + root: 'cursor-pointer flex-col items-start rounded-xl border-none bg-gray-100 p-6 text-left text-lg shadow-md hover:bg-gray-200 transition-colors', + } - className={{ - root: 'cursor-pointer flex-col items-start rounded-xl border-none bg-gray-100 p-6 text-left text-lg shadow-md', - }} + className={BUTTON_STYLES}Also applies to: 31-42
47-48: Choose a single export style.The component has both named and default exports. Consider sticking to one style to maintain consistency and prevent confusion.
export function CTA() { // ... component implementation } - export default CTAapps/docs/package.json (1)
25-25: Consider using consistent version range notation.While the addition of @types/react is good for TypeScript support, consider using the tilde (~) version range for consistency with other dependencies.
- "@types/react": "^18.3.11", + "@types/react": "~18.3.11",apps/docs/src/components/landing/FeatureFocusSection.tsx (2)
14-55: Enhance accessibility and image handling.Several improvements could be made to enhance accessibility and user experience:
- The image's alt text is generic ("App screenshot"). Consider making it customizable through props.
- The gradient overlay div marked as
aria-hidden="true"is good, but ensure it doesn't contain any meaningful content.- Consider adding ARIA labels for the features section to improve screen reader navigation.
interface Props { title: string description: string features: { title: string; icon: React.ReactNode; text: string }[] imgSrc?: string + imgAlt?: string } // In the component: - alt="App screenshot" + alt={imgAlt ?? "App screenshot"} // Add ARIA label to features section: - <dl className="mx-auto grid max-w-2xl grid-cols-1 gap-x-6 gap-y-10 + <dl aria-label="Feature list" className="mx-auto grid max-w-2xl grid-cols-1 gap-x-6 gap-y-10
56-56: Consider using only named exports for consistency.The component is currently exported both as a named export and a default export. To maintain consistency and avoid confusion, consider sticking to named exports throughout the codebase.
-export default FeatureFocusSectionapps/docs/src/components/landing/FeatureSection.tsx (3)
6-19: Add JSDoc comments to interfaces for better documentation.While the interfaces are well-structured, adding JSDoc comments would improve code documentation and IDE support.
+/** + * Represents a feature item with its associated properties + */ interface Feature { title: string icon: IconDefinition text: string hoverImage: string shadow?: boolean isComingSoon?: boolean } +/** + * Props for the FeatureSection component + */ interface FeatureSectionProps { title: string description: string features: Feature[] }
21-23: Consider memoizing the hover handler for better performance.The hover handler function is recreated on every render. Consider using useCallback to optimize performance.
- const [hoveredFeatureIx, setHoveredFeatureIx] = useState<number>(0) + const [hoveredFeatureIx, setHoveredFeatureIx] = useState<number>(0) + const handleMouseEnter = useCallback((ix: number) => setHoveredFeatureIx(ix), [])
33-55: Enhance accessibility for the feature list.The feature list could benefit from improved semantic markup and ARIA attributes for better screen reader support.
-<dl className="max-w-xl flex-1 space-y-6 text-base text-gray-600 lg:max-w-none"> +<dl className="max-w-xl flex-1 space-y-6 text-base text-gray-600 lg:max-w-none" role="list"> {features.map((feature, ix) => ( <div key={feature.title} className={twMerge( 'flex cursor-pointer flex-row items-center gap-6 p-6 pl-9', hoveredFeatureIx === ix && 'rounded-xl bg-gray-100' )} onMouseEnter={() => setHoveredFeatureIx(ix)} + role="listitem" + aria-label={`Feature: ${feature.title}`} >apps/docs/src/components/landing/TitleImage.tsx (2)
20-33: Add security attributes to external link and move inline styles to CSS.The external link should include
rel="noopener noreferrer"for security, and inline styles should be moved to CSS classes for better maintainability.<a href="https://community.klicker.uzh.ch/t/klickeruzh-v3-2-release-information/388" - className="whitespace-nowrap font-semibold" + className="whitespace-nowrap font-semibold ml-3" target="_blank" + rel="noopener noreferrer" - style={{ marginLeft: '0.75rem' }} >
56-66: Enhance security and simplify string concatenation.Add security attributes to the external link and consider using template literals for better readability.
- We are now regularly offering introductory courses through UZH - Central IT. For more details see{' '} + We are now regularly offering introductory courses through UZH + Central IT. For more details see{' '} <a target="_blank" + rel="noopener noreferrer" href="https://community.klicker.uzh.ch/t/2024-01-10-2024-02-08-klickeruzh-v3-0-introduction-and-didactic-use-cases/257" >apps/docs/src/pages/index.tsx (3)
24-24: Standardize image paths for better maintainability.The image paths show inconsistency in naming convention:
- Some use
img_v3/XX_name.png- Others use just
img_v3/name.png- Some include additional subdirectories
Consider standardizing the image path structure for better maintainability.
Also applies to: 30-30, 43-43, 49-49, 55-55, 68-68, 75-75
14-78: Consider layout improvements for feature sections.Based on previous feedback, consider:
- Adjusting the spacing between text blocks for better visual flow
- Experimenting with alternating image sizes between sections (like gbl.uzh.ch) to create visual interest
80-103: Document future features in a separate tracking system.Instead of keeping future features commented in the code, consider:
- Moving them to a product roadmap or issue tracker
- Adding a TODO comment with a reference to the tracking item
🛑 Comments failed to post (7)
apps/docs/src/components/landing/UseCaseOverview.tsx (1)
21-28: 🛠️ Refactor suggestion
Enhance accessibility of the "More Details" link.
The generic "More Details" text might not be descriptive enough for screen readers. Consider including the use case title in the link text.
<a className="inline-flex items-center gap-2" href={`/use_cases/${href}`} + aria-label={`More details about ${item.title}`} > <FontAwesomeIcon icon={faArrowRight} /> - <div>More Details</div> + <div>More Details<span className="sr-only"> about {item.title}</span></div> </a>Committable suggestion skipped: line range outside the PR's diff.
apps/docs/src/components/teampage/team.tsx (2)
24-30: 🛠️ Refactor suggestion
Improve image handling and prevent layout shifts.
The current image implementation could benefit from:
- Image optimization
- Proper error handling
- Loading states
Consider using Next.js Image component or implement proper image handling:
<div className="h-52 overflow-hidden bg-white p-2 pt-3"> - <img - src={item.imgSrc} - alt={item.name} - className="h-full w-full transform object-contain" - /> + <Image + src={item.imgSrc} + alt={item.name} + width={200} + height={200} + className="h-full w-full transform object-contain" + loading="lazy" + onError={(e) => { + e.currentTarget.src = '/placeholder-avatar.png' + }} + /> </div>Committable suggestion skipped: line range outside the PR's diff.
9-43:
⚠️ Potential issueAdd accessibility features and improve component robustness.
Several improvements are needed:
- Avoid using array index as key in the map function
- Add proper ARIA labels and roles
- Handle empty states and loading
Apply these changes:
-function Team({ teamMembers }: { teamMembers: TeamMember[] }) { +function Team({ teamMembers, isLoading }: { teamMembers: TeamMember[], isLoading?: boolean }) { return ( - <section className="pt-12"> + <section className="pt-12" aria-labelledby="team-heading"> <div className="container mx-auto"> <div className="mb-12 text-center"> - <h2 className="mt-2 text-3xl font-bold tracking-tight text-gray-900 sm:text-4xl"> + <h2 id="team-heading" className="mt-2 text-3xl font-bold tracking-tight text-gray-900 sm:text-4xl"> Our Team </h2> </div> + {isLoading && <div>Loading team members...</div>} + {!isLoading && teamMembers.length === 0 && ( + <p className="text-center text-gray-600">No team members to display.</p> + )} <div className="flex flex-wrap justify-center gap-8"> {teamMembers.map((item, index) => ( <div className="w-16 overflow-hidden rounded-lg shadow-lg sm:w-1/2 md:w-1/3 lg:w-1/4" - key={index} + key={`${item.name}-${item.position}`} + role="article" + aria-label={`Team member ${item.name}`} >📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.function Team({ teamMembers, isLoading }: { teamMembers: TeamMember[], isLoading?: boolean }) { return ( <section className="pt-12" aria-labelledby="team-heading"> <div className="container mx-auto"> <div className="mb-12 text-center"> <h2 id="team-heading" className="mt-2 text-3xl font-bold tracking-tight text-gray-900 sm:text-4xl"> Our Team </h2> </div> {isLoading && <div>Loading team members...</div>} {!isLoading && teamMembers.length === 0 && ( <p className="text-center text-gray-600">No team members to display.</p> )} <div className="flex flex-wrap justify-center gap-8"> {teamMembers.map((item, index) => ( <div className="w-16 overflow-hidden rounded-lg shadow-lg sm:w-1/2 md:w-1/3 lg:w-1/4" key={`${item.name}-${item.position}`} role="article" aria-label={`Team member ${item.name}`} > <div className="h-52 overflow-hidden bg-white p-2 pt-3"> <img src={item.imgSrc} alt={item.name} className="h-full w-full transform object-contain" /> </div> <div className="pb-0 pt-2 text-center"> <h4 className="text-primary mb-2 text-xl font-medium"> {item.name} </h4> <p className="mb-2 text-sm text-gray-600">{item.position}</p> </div> </div> ))} </div> </div> </section> ) }apps/docs/src/components/landing/FeatureFocusSection.tsx (1)
1-6: 🛠️ Refactor suggestion
Improve type safety and documentation of Props interface.
- Replace the
anytype foriconwith a more specific type to improve type safety.- Consider adding JSDoc documentation to describe the purpose of each prop.
+/** + * Props for the FeatureFocusSection component + * @property {string} title - The main title of the section + * @property {string} description - The description text below the title + * @property {Feature[]} features - Array of features to display + * @property {string} [imgSrc] - Optional URL for the section image + */ interface Props { title: string description: string - features: { title: string; icon: any; text: string }[] + features: { title: string; icon: React.ReactNode; text: string }[] imgSrc?: string }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements./** * Props for the FeatureFocusSection component * @property {string} title - The main title of the section * @property {string} description - The description text below the title * @property {Feature[]} features - Array of features to display * @property {string} [imgSrc] - Optional URL for the section image */ interface Props { title: string description: string features: { title: string; icon: React.ReactNode; text: string }[] imgSrc?: string }apps/docs/src/components/landing/FeatureSection.tsx (1)
57-63: 🛠️ Refactor suggestion
Enhance image handling and accessibility.
The current image implementation could be improved in several ways:
- Add error handling for invalid image URLs
- Provide more descriptive alt text
- Add loading state handling
<div className="hidden flex-1 pt-4 sm:block"> <img src={features[hoveredFeatureIx].hoverImage ?? ''} - alt="Feature specific screenshot" + alt={`Screenshot demonstrating ${features[hoveredFeatureIx].title}`} + onError={(e) => { + e.currentTarget.src = '/fallback-image.png' + console.error(`Failed to load image for feature: ${features[hoveredFeatureIx].title}`) + }} className="h-auto max-h-[400px] w-full object-contain" + loading="lazy" /> </div>Committable suggestion skipped: line range outside the PR's diff.
apps/docs/src/components/landing/TitleImage.tsx (1)
35-55: 🛠️ Refactor suggestion
Improve accessibility and security attributes.
The logo image needs an alt text for accessibility, and the external link should include security attributes.
- <img className="-ml-2 w-80" src="/img/KlickerLogo.png" /> + <img className="-ml-2 w-80" src="/img/KlickerLogo.png" alt="KlickerUZH Logo" /> - <a href="https://manage.klicker.uzh.ch" target="_blank"> + <a href="https://manage.klicker.uzh.ch" target="_blank" rel="noopener noreferrer">📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.<img className="-ml-2 w-80" src="/img/KlickerLogo.png" alt="KlickerUZH Logo" /> <p className="mt-6 text-2xl leading-8 text-gray-600"> Enhance your classroom experience. </p> <div className="mt-10 flex items-center gap-x-6"> <a href="https://manage.klicker.uzh.ch" target="_blank" rel="noopener noreferrer"> <Button className={{ root: 'border-uzh-blue-40 w-full cursor-pointer text-xl md:w-max', }} > Sign Up / Login </Button> </a> <a href="/getting_started/welcome" className="font-semibold leading-6 text-gray-900" > Get started <span aria-hidden="true">→</span> </a> </div>apps/docs/src/pages/index.tsx (1)
127-142:
⚠️ Potential issueSecurity: Remove hardcoded personal image URLs.
The commented Team component contains direct URLs to personal photos on df.uzh.ch. This could lead to:
- Privacy concerns if the URLs contain personal information
- Broken images if the source URLs change
- Potential security risks if the source domain's security policies change
Consider:
- Moving images to your project's assets
- Using placeholder images during development
…ication_docs # Conflicts: # apps/docs/package.json # pnpm-lock.yaml
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 1509424 | Triggered | Generic Password | 5de5d7c | docker-compose.yml | View secret |
| 1509424 | Triggered | Generic Password | 5de5d7c | util/_restore-prod.sh | View secret |
| 1509424 | Triggered | Generic Password | 5de5d7c | util/_restore-prod.sh | View secret |
| 1509424 | Triggered | Generic Password | 5de5d7c | util/_restore-prod.sh | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
|
klicker-uzh
|
||||||||||||||||||||||||||||
| Project |
klicker-uzh
|
| Branch Review |
v3
|
| Run status |
|
| Run duration | 11m 20s |
| Commit |
|
| Committer | Chris |
| View all properties for this run ↗︎ | |
| Test results | |
|---|---|
|
Failures
|
0
|
|
Flaky
|
0
|
|
Pending
|
0
|
|
Skipped
|
0
|
|
Passing
|
140
|
| View all changes introduced in this branch ↗︎ | |
Summary by CodeRabbit
Release Notes
New Features
FeatureFocusSection,FeatureSection,TitleImage,UseCaseOverview, andCTA.Updates
Bug Fixes
Chores