Polish mobile carousel: reset animations on swipe, improve arrows, distinct bg#5
Conversation
…stinct bg - Add isActive prop to all 5 demo components so animations reset and replay from the beginning when a slide becomes visible via swipe - Make navigation arrows always visible (opacity-30 on edges instead of hidden), larger (w-10 h-10), solid background, stronger shadow - Change section background from bg-muted/20 to bg-muted for visual separation from adjacent sections Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @kushal238, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refines the user experience of the feature showcase section, particularly on mobile devices. It introduces a dynamic carousel for mobile, ensuring smoother transitions and clearer navigation between features. Additionally, it addresses animation behavior to provide a consistent and engaging presentation of each feature, while also making minor visual and layout improvements for both mobile and desktop views. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request significantly improves the mobile user experience by replacing the expandable cards with a swipeable carousel for the feature showcase. The changes include resetting animations on swipe using an isActive prop, enhancing the navigation arrows, and adjusting the background for better visual distinction. The implementation is solid, and my feedback focuses on further polishing the code for better maintainability, readability, and accessibility. I've suggested creating a reusable type for component props, simplifying some logic and markup, improving button accessibility, and adhering more closely to common React patterns for rendering components.
| <button | ||
| key={index} | ||
| onClick={() => scrollToSlide(index)} | ||
| className={`rounded-full transition-all duration-300 ${ | ||
| mobileActive === index | ||
| ? "w-6 h-2.5 bg-primary" | ||
| : "w-2.5 h-2.5 bg-border hover:bg-muted-foreground/50" | ||
| }`} | ||
| /> |
There was a problem hiding this comment.
These buttons are missing type="button" and an accessible name. For accessibility, buttons without visible text should have an aria-label to describe their purpose to screen reader users. Also, adding type="button" is a good practice to prevent unintended form submissions.
Example:
<button
type="button"
key={index}
onClick={() => scrollToSlide(index)}
aria-label={`Go to slide ${index + 1}`}
// ... className
/>|
|
||
| // Smart Dictation Demo Component | ||
| const SmartDictationDemo = () => { | ||
| const SmartDictationDemo = ({ compact, isActive }: { compact?: boolean; isActive?: boolean }) => { |
There was a problem hiding this comment.
The props type { compact?: boolean; isActive?: boolean } is repeated across all five demo components (e.g., SmartDictationDemo, SmartMedicationSearchDemo, etc.) and in the Feature interface (line 1021). To improve reusability and maintainability, consider defining a shared type alias:
type DemoComponentProps = { compact?: boolean; isActive?: boolean };You can then use this type in all component signatures, for example:
const SmartDictationDemo = ({ compact, isActive }: DemoComponentProps) => { ... }
And in the Feature interface:
component: React.ComponentType<DemoComponentProps>;
| "Patient complains of fever and headache for 3 days. Prescribing Paracetamol 500mg and Cetirizine 10mg for symptom relief."; | ||
|
|
||
| useEffect(() => { | ||
| if (isActive === false) { |
There was a problem hiding this comment.
For conciseness and idiomatic code, you can use the logical NOT operator ! to check for a false value. This pattern is repeated in the useEffect hooks of the other demo components as well.
| if (isActive === false) { | |
| if (!isActive) { |
|
|
||
| {compact ? ( | ||
| <div className="flex flex-wrap gap-x-3 gap-y-1 text-xs text-muted-foreground"> | ||
| <span><span className="font-medium text-foreground">Hypertension</span></span> |
There was a problem hiding this comment.
The outer <span> element appears to be unnecessary here as it doesn't have any attributes and only wraps another <span>. You can remove it for cleaner markup.
| <span><span className="font-medium text-foreground">Hypertension</span></span> | |
| <span className="font-medium text-foreground">Hypertension</span> |
| {(() => { | ||
| const IconComp = features[mobileActive].icon; | ||
| return <IconComp className="w-5 h-5 text-white" />; | ||
| })()} |
There was a problem hiding this comment.
While this IIFE (Immediately Invoked Function Expression) works for rendering a dynamic component, a more conventional and readable approach in React is to define the component in a variable before the return statement. This pattern is also used on line 1327.
For example:
const IconComp = features[mobileActive].icon;
// ... in JSX
<div ...>
<IconComp className="w-5 h-5 text-white" />
</div>| <button | ||
| onClick={() => scrollToSlide(mobileActive - 1)} | ||
| className={`absolute left-2 top-1/2 -translate-y-1/2 w-10 h-10 rounded-full bg-background flex items-center justify-center border border-border shadow-md z-10 ${ | ||
| mobileActive === 0 ? "opacity-30 pointer-events-none" : "" | ||
| }`} | ||
| aria-label="Previous feature" | ||
| > | ||
| <ChevronLeft className="w-5 h-5 text-foreground" /> | ||
| </button> |
There was a problem hiding this comment.
It's a good practice to add type="button" to buttons to prevent them from submitting a form if they happen to be rendered inside one. This also applies to the 'Next feature' button.
| {(() => { | ||
| const DemoComponent = features[activeFeature].component; | ||
| return <DemoComponent />; | ||
| })()} |
There was a problem hiding this comment.
This IIFE (Immediately Invoked Function Expression) can be simplified. A more conventional approach in React is to assign the component to a variable before the return statement and then render it. This improves readability.
For example:
const DemoComponent = features[activeFeature].component;
// ... in your JSX ...
<DemoComponent />Since ActiveDemoComponent was removed earlier, you can re-introduce it or a similar variable before the main return statement of FeatureShowcase.
- Simulate press (scale 0.95) on prescription card before expanding - Change carousel slide from overflow-hidden to overflow-y-auto so tall content doesn't clip at the top - Extract shared DemoComponentProps type for all 5 demo components - Add type="button" and aria-label to progress dot buttons (a11y) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request refactors the FeatureShowcase component, primarily by replacing the mobile expandable card layout with an Instagram-style swipeable carousel. This involved introducing compact and isActive props to all individual demo components (SmartDictationDemo, SmartMedicationSearchDemo, AggregatedHistoryDemo, ComplaintBasedDiagnosisDemo, PatientRAGChatbotDemo) to enable state resetting when inactive and to adjust their styling for the new compact mobile view. The desktop layout was also updated to reorder the demo component and feature description. Review comments suggest improving boolean checks to !isActive for conciseness and refactoring verbose Immediately Invoked Function Expressions (IIFEs) used for rendering icon components into a more idiomatic React pattern.
| if (isActive === false) { | ||
| setText(""); | ||
| setHighlights([]); | ||
| return; | ||
| } |
There was a problem hiding this comment.
For boolean checks, it's more idiomatic in JavaScript/TypeScript to use the negation operator !. This makes the code slightly more concise and is a common convention. This feedback applies to the other similar checks in this file.
| if (isActive === false) { | |
| setText(""); | |
| setHighlights([]); | |
| return; | |
| } | |
| if (!isActive) { | |
| setText(""); | |
| setHighlights([]); | |
| return; | |
| } |
| {(() => { | ||
| const IconComp = features[mobileActive].icon; | ||
| return <IconComp className="w-5 h-5 text-white" />; | ||
| })()} |
There was a problem hiding this comment.
This IIFE (Immediately Invoked Function Expression) to render the icon component is a bit verbose and not a common pattern in React. For better readability, you could destructure icon into a PascalCased variable (e.g., const { icon: Icon } = features[mobileActive]) before this block and then use <Icon ... /> directly. This would make the JSX cleaner. This pattern is used in a few other places in this file (e.g., lines 1314, 1341) and could be refactored for consistency.
1 participant
Was needed to fix the feature card issue on mobile.
Add isActive prop to all 5 demo components so animations reset and replay from the beginning when a slide becomes visible via swipe
Make navigation arrows always visible (opacity-30 on edges instead of hidden), larger (w-10 h-10), solid background, stronger shadow
Change section background from bg-muted/20 to bg-muted for visual separation from adjacent sections