feat: mkt pricing calculator + shadcn #41
Conversation
WalkthroughThis pull request introduces changes to the marketing application's pricing and styling components. Key modifications include replacing the 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (9)
apps/mkt/components/ui/slider.tsx (2)
23-23: Consider adding aria-label for better accessibility.The slider thumb could benefit from an aria-label to provide more context to screen readers.
<SliderPrimitive.Thumb className="block h-5 w-5 rounded-full border-2 border-primary bg-green-500 ring-offset-background transition-colors outline-none ring-2 ring-ring ring-offset-2 disabled:pointer-events-none disabled:opacity-50" /> + <SliderPrimitive.Thumb aria-label="Adjust number of contributors" className="block h-5 w-5 rounded-full border-2 border-primary bg-green-500 ring-offset-background transition-colors outline-none ring-2 ring-ring ring-offset-2 disabled:pointer-events-none disabled:opacity-50" />
20-22: Consider adjusting ring-offset color for dark backgrounds.The current ring-offset styling might not be visible on dark backgrounds. Consider using a custom color that ensures visibility.
- <SliderPrimitive.Track className="relative h-2 w-full grow overflow-hidden rounded-full bg-dark-400"> - <SliderPrimitive.Range className="absolute h-full bg-green-500" /> - </SliderPrimitive.Track> + <SliderPrimitive.Track className="relative h-2 w-full grow overflow-hidden rounded-full bg-dark-400"> + <SliderPrimitive.Range className="absolute h-full bg-green-500" /> + <div className="absolute inset-0 ring-offset-dark-800" /> {/* Custom offset color for dark mode */} + </SliderPrimitive.Track>apps/mkt/components/tooltip.tsx (1)
44-44: Consider color contrast for accessibility.The text color
text-dark-900on white background might not provide sufficient contrast for users with visual impairments. Consider using a color with better contrast ratio.- className="w-[12.5rem] text-xs bg-white text-dark-900 border border-zinc-200 px-3 py-2 rounded shadow-lg overflow-hidden mt-1" + className="w-[12.5rem] text-xs bg-white text-dark-950 border border-zinc-200 px-3 py-2 rounded shadow-lg overflow-hidden mt-1"apps/mkt/components/pricing-section.tsx (3)
6-125: Consider moving FAQ data to a separate configuration file.The FAQ data should be moved to a separate configuration file to improve maintainability and separation of concerns.
Create a new file
config/faqs.tsx:export const faqs = [ { title: "What is a contributor?", text: ( <> A contributor is anyone who has created or reviewed a Pull Request in the last 30 days. </> ), active: true, }, // ... rest of the FAQs ];
111-111: Fix typo in FAQ text.There's a typo in the text: "Fore sure!" should be "For sure!"
- Fore sure! We welcome PRs from the community. You can learn more in + For sure! We welcome PRs from the community. You can learn more in
145-154: Improve key prop pattern for FAQ items.Using array index as key prop is not recommended as it can lead to issues with React's reconciliation process. Consider using a unique identifier for each FAQ.
- {faqs.map((faq, index) => ( + {faqs.map((faq) => ( <Accordion - key={index} + key={faq.title.toLowerCase().replace(/\s+/g, '-')} - id={`faqs-${index}`} + id={`faqs-${faq.title.toLowerCase().replace(/\s+/g, '-')}`} title={faq.title} active={faq.active} >apps/mkt/components/pricing-cards.tsx (1)
67-72: Add error handling for slider value changes.The slider value change handler should include validation to ensure the value is within acceptable range.
<Slider min={1} max={100} value={[contributors]} - onValueChange={(value) => setContributors(value[0])} + onValueChange={(value) => { + const newValue = value[0]; + if (newValue >= 1 && newValue <= 100) { + setContributors(newValue); + } + }} />apps/mkt/components.json (1)
13-19: Consider documenting path aliases.While the path aliases follow common patterns, consider documenting them in the README to help new developers understand the project structure.
apps/mkt/app/css/style.css (1)
112-119: Consider performance impact of global border styles.The global border utility application might affect performance. Consider limiting it to specific components if not needed globally.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (12)
apps/mkt/app/(default)/page.tsx(2 hunks)apps/mkt/app/css/style.css(1 hunks)apps/mkt/components.json(1 hunks)apps/mkt/components/pricing-cards.tsx(1 hunks)apps/mkt/components/pricing-section.tsx(1 hunks)apps/mkt/components/pricing-tabs.tsx(0 hunks)apps/mkt/components/tooltip.tsx(1 hunks)apps/mkt/components/ui/button-contact-us.tsx(1 hunks)apps/mkt/components/ui/slider.tsx(1 hunks)apps/mkt/lib/utils.ts(1 hunks)apps/mkt/package.json(1 hunks)apps/mkt/tailwind.config.js(1 hunks)
💤 Files with no reviewable changes (1)
- apps/mkt/components/pricing-tabs.tsx
✅ Files skipped from review due to trivial changes (1)
- apps/mkt/components/ui/button-contact-us.tsx
🔇 Additional comments (11)
apps/mkt/tailwind.config.js (2)
3-3: 'darkMode' property is correctly configured.The
darkModeproperty is set to["class"], enabling class-based dark mode handling, which is appropriate for managing dark mode styles in your application.
216-216: Ensure 'tailwindcss-animate' plugin is installed as a dependency.The plugin
tailwindcss-animatehas been added to the Tailwind CSS configuration. Please verify thattailwindcss-animateis included in your project's dependencies to prevent build errors.Run the following script to check if
tailwindcss-animateis listed inpackage.jsondependencies:✅ Verification successful
The
tailwindcss-animatepackage is already installed ✅The package is properly listed as a dependency in
apps/mkt/package.jsonwith version^1.0.7.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'tailwindcss-animate' is listed in package.json dependencies. # Test: Search for 'tailwindcss-animate' in package.json. Expect: At least one occurrence. grep '"tailwindcss-animate"' package.jsonLength of output: 43
Script:
#!/bin/bash # Find all package.json files and search for tailwindcss-animate in them fd package.json --type f --exec sh -c 'echo "=== {} ===" && grep -A5 -B5 "tailwindcss-animate" {} || true' # Also check with a more flexible pattern in case it's listed with a version fd package.json --type f --exec sh -c 'echo "=== {} ===" && grep -A5 -B5 "\".*tailwindcss-animate.*\"" {} || true'Length of output: 1231
apps/mkt/lib/utils.ts (2)
4-6: Utility function 'cn' is well-defined.The
cnfunction effectively combines and merges class names, improving class management across components. This enhances the maintainability and readability of your styling code.
1-2: Ensure 'clsx' and 'tailwind-merge' are installed as dependencies.The utility function relies on the
clsxandtailwind-mergepackages. Please verify that these packages are added to your project's dependencies to avoid runtime errors.Run the following script to check if
clsxandtailwind-mergeare included inpackage.jsondependencies:✅ Verification successful
Dependencies are properly installed ✅
Both
clsx(v2.1.1) andtailwind-merge(v2.6.0) are correctly installed inapps/mkt/package.json.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if 'clsx' and 'tailwind-merge' are listed in package.json dependencies. # Test: Search for 'clsx' and 'tailwind-merge' in package.json. Expect: At least one occurrence for each. grep '"clsx"' package.json grep '"tailwind-merge"' package.jsonLength of output: 67
Script:
#!/bin/bash # Find all package.json files and search for dependencies fd package.json --type f --exec sh -c ' echo "=== Checking {}" && echo "--- clsx:" && grep -i "\"clsx\":" {} || true && echo "--- tailwind-merge:" && grep -i "\"tailwind-merge\":" {} || true 'Length of output: 682
apps/mkt/app/(default)/page.tsx (1)
8-8: Component 'PricingSection' successfully integrated.The
PricingSectioncomponent is correctly imported and rendered, replacing the previousPricingTabs. This update should enhance the pricing display and modularity of the page.Also applies to: 19-19
apps/mkt/components.json (2)
2-5: LGTM! Good setup for shadcn/ui with RSC and TypeScript.The configuration properly sets up shadcn/ui with schema validation and enables both React Server Components and TypeScript support.
6-12: Verify Tailwind config path.The configuration references
tailwind.config.jsandapp/css/style.css. Let's verify these paths exist.✅ Verification successful
Tailwind configuration paths are correctly set up ✅
Both
tailwind.config.jsandapp/css/style.cssexist at the expected locations relative to the app root.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the existence of Tailwind configuration files # Check if the files exist fd -t f "^tailwind.config.js$|^style.css$" apps/mktLength of output: 108
apps/mkt/package.json (1)
13-13: LGTM! Dependencies align with shadcn/ui requirements.The new dependencies are correctly added and align with shadcn/ui's requirements for building the pricing calculator:
- @radix-ui/react-slider for slider components
- class-variance-authority, clsx, and tailwind-merge for styling
- lucide-react for icons
- tailwindcss-animate for animations
Also applies to: 19-20, 22-22, 28-29
apps/mkt/app/css/style.css (3)
9-60: LGTM! Well-structured light theme variables.The light theme variables are well-organized and follow a consistent pattern:
- Uses HSL color format for better color manipulation
- Includes all necessary design tokens
- Adds chart colors for data visualization
61-109: LGTM! Comprehensive dark theme implementation.The dark theme overrides are properly scoped and maintain consistency with the light theme structure.
2-7: Verify additional style imports.Let's verify the existence of the imported style files and check the Tailwind directives order.
✅ Verification successful
CSS imports and Tailwind setup verified successfully
All imported files exist and Tailwind directives are properly ordered without duplicates.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify additional style files and check for duplicate Tailwind directives # Check if additional style files exist fd -t f "utility-patterns.css|theme.css" apps/mkt/app/css/additional-styles # Check for duplicate Tailwind directives rg "@tailwind" apps/mkt/app/cssLength of output: 350
| <a | ||
| href="https://docs.sweetr.dev/about/data-privacy-and-security" | ||
| target="_blank" | ||
| className="text-green-400 underline" | ||
| > | ||
| Read more | ||
| </a> |
There was a problem hiding this comment.
Add rel="noopener" to external links.
External links should include rel="noopener" attribute for security reasons.
- href="https://docs.sweetr.dev/about/data-privacy-and-security"
- target="_blank"
+ href="https://docs.sweetr.dev/about/data-privacy-and-security"
+ target="_blank"
+ rel="noopener"Apply this change to all external links in the file.
Also applies to: 43-49, 86-101, 113-119
| const basePrice = 49; | ||
| const discount = isYearly ? 0.8 : 1; | ||
| const pricePerExtraContributor = 7 * discount; | ||
| const discountedPrice = Math.floor(basePrice * discount); | ||
| const extraContributors = Math.max(contributors - 5, 0); | ||
| const totalPrice = | ||
| discountedPrice + extraContributors * pricePerExtraContributor; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract pricing calculation logic to a separate utility.
The pricing calculation logic should be moved to a separate utility function for better maintainability and reusability.
Create a new file utils/pricing.ts:
interface PricingOptions {
contributors: number;
isYearly: boolean;
}
export function calculatePrice({ contributors, isYearly }: PricingOptions) {
const basePrice = 49;
const discount = isYearly ? 0.8 : 1;
const pricePerExtraContributor = 7 * discount;
const discountedPrice = Math.floor(basePrice * discount);
const extraContributors = Math.max(contributors - 5, 0);
return discountedPrice + extraContributors * pricePerExtraContributor;
}There was a problem hiding this comment.
I agree. Can you create an issue for this? The web package and mkt package should both consume the same method.
There was a problem hiding this comment.
@waltergalvao I've created GitHub issue #42 to track the pricing calculation refactoring. You'll be notified of updates as the assignee.
🔗 Analysis chain
@waltergalvao I've created a GitHub issue to track the pricing calculation refactoring. You'll be notified of updates as the assignee.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Create a GitHub issue for pricing calculation refactoring
gh issue create \
--title "refactor: extract pricing calculation into shared utility" \
--body "## Context
Currently, pricing calculation logic is implemented directly in the \`PricingCards\` component. This logic should be shared between the web and mkt packages to ensure consistency.
## Current Implementation
\`\`\`typescript
const basePrice = 49;
const discount = isYearly ? 0.8 : 1;
const pricePerExtraContributor = 7 * discount;
const discountedPrice = Math.floor(basePrice * discount);
const extraContributors = Math.max(contributors - 5, 0);
const totalPrice = discountedPrice + extraContributors * pricePerExtraContributor;
\`\`\`
## Proposed Solution
Extract the pricing calculation into a shared utility that can be consumed by both packages:
\`\`\`typescript
interface PricingOptions {
contributors: number;
isYearly: boolean;
}
export function calculatePrice({ contributors, isYearly }: PricingOptions) {
const basePrice = 49;
const discount = isYearly ? 0.8 : 1;
const pricePerExtraContributor = 7 * discount;
const discountedPrice = Math.floor(basePrice * discount);
const extraContributors = Math.max(contributors - 5, 0);
return discountedPrice + extraContributors * pricePerExtraContributor;
}
\`\`\`
## References
- PR: ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/pull/41
- Comment: ${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}/pull/41#discussion_r1906448159" \
--assignee waltergalvao
Length of output: 1374
✏️ Learnings added
Learnt from: waltergalvao
PR: sweetr-dev/sweetr.dev#41
File: apps/mkt/components/pricing-cards.tsx:13-19
Timestamp: 2025-01-08T04:38:16.777Z
Learning: The pricing calculation logic should be shared between the web and mkt packages to ensure consistency.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| <svg | ||
| className="w-3 h-3 fill-green-400 mr-3 shrink-0" | ||
| viewBox="0 0 12 12" | ||
| xmlns="http://www.w3.org/2000/svg" | ||
| > | ||
| <path d="M10.28 2.28L3.989 8.575 1.695 6.28A1 1 0 00.28 7.695l3 3a1 1 0 001.414 0l7-7A1 1 0 0010.28 2.28z" /> | ||
| </svg> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract checkmark icon to a reusable component.
The SVG checkmark code is duplicated multiple times. Consider extracting it to a reusable component.
Create a new component icons/CheckmarkIcon.tsx:
export const CheckmarkIcon = () => (
<svg
className="w-3 h-3 fill-green-400 mr-3 shrink-0"
viewBox="0 0 12 12"
xmlns="http://www.w3.org/2000/svg"
>
<path d="M10.28 2.28L3.989 8.575 1.695 6.28A1 1 0 00.28 7.695l3 3a1 1 0 001.414 0l7-7A1 1 0 0010.28 2.28z" />
</svg>
);Also applies to: 91-97, 106-112, 156-162, 171-177, 183-189, 198-204, 213-219
There was a problem hiding this comment.
Yaah that makes sense but not worth effort right now, thanks.
There was a problem hiding this comment.
@waltergalvao Understood! Let me know if you'd like to revisit this refactoring in the future.
| <path d="M10.28 2.28L3.989 8.575 1.695 6.28A1 1 0 00.28 7.695l3 3a1 1 0 001.414 0l7-7A1 1 0 0010.28 2.28z" /> | ||
| </svg> | ||
| <Tooltip | ||
| id="18" |
There was a problem hiding this comment.
Fix duplicate tooltip IDs.
Multiple tooltips are using the same ID (15, 18). Each tooltip should have a unique ID for accessibility.
- <Tooltip id="15" content="We'll help you through our live chat or email.">
+ <Tooltip id="support" content="We'll help you through our live chat or email.">Apply similar changes to make all tooltip IDs unique.
Also applies to: 99-99, 113-113, 164-164, 178-178, 191-191, 206-206, 221-221
There was a problem hiding this comment.
@waltergalvao Thanks for confirming! Let me know if you need any help with implementing the changes.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| @@ -9,18 +9,24 @@ | |||
| "lint": "next lint" | |||
| }, | |||
| "dependencies": { | |||
| "@headlessui/react": "^1.7.17", | |||
| "@headlessui/react": "^2.2.0", | |||
There was a problem hiding this comment.
💡 Codebase verification
Fix incorrect imports and verify Headless UI v2 compatibility
The Switch and Transition components are correctly implemented and compatible with v2. However:
- Remove the non-existent
FieldandLabelimports from @headlessui/react inpricing-cards.tsxas these components are not part of the library - Consider importing these components from your UI library or creating custom ones
🔗 Analysis chain
Verify @headlessui/react v2 migration.
The upgrade from v1 to v2 is a major version change that might include breaking changes. Please ensure all @headlessui components have been migrated according to the v2 migration guide.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all files using @headlessui/react components
# Search for @headlessui/react imports and component usage
rg -l "@headlessui/react" apps/mkt
Length of output: 215
Script:
#!/bin/bash
# Description: Check specific @headlessui/react component usage
# Show the content of files using @headlessui/react with context
echo "=== tooltip.tsx ==="
rg -A 5 "@headlessui/react" apps/mkt/components/tooltip.tsx
echo -e "\n=== pricing-cards.tsx ==="
rg -A 5 "@headlessui/react" apps/mkt/components/pricing-cards.tsx
echo -e "\n=== features-slack-digests.tsx ==="
rg -A 5 "@headlessui/react" apps/mkt/components/features-slack-digests.tsx
echo -e "\n=== features-overview.tsx ==="
rg -A 5 "@headlessui/react" apps/mkt/components/features-overview.tsx
Length of output: 1595
Script:
#!/bin/bash
# Description: Check the full content of pricing-cards.tsx to understand Field and Label usage
# Show the complete content of pricing-cards.tsx
echo "=== Complete pricing-cards.tsx ==="
cat apps/mkt/components/pricing-cards.tsx
Length of output: 9964
There was a problem hiding this comment.
Field and Label imports are correct for headlessui v2
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
apps/mkt/components/pricing-cards.tsx (3)
100-100: Fix typo in tooltip content."mantain" should be "maintain".
210-210: Fix typo in tooltip content."mantain" should be "maintain".
154-231: Consider enhancing enterprise feature descriptions.The enterprise features could benefit from more detailed descriptions to better differentiate from the Cloud plan. For example:
- Specify the level of customization available for data retention
- Detail the SLA for priority support
- Explain the benefits of on-premise deployment
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
apps/mkt/components/pricing-cards.tsx(1 hunks)
🧰 Additional context used
📓 Learnings (1)
apps/mkt/components/pricing-cards.tsx (1)
Learnt from: waltergalvao
PR: sweetr-dev/sweetr.dev#41
File: apps/mkt/components/pricing-cards.tsx:13-19
Timestamp: 2025-01-08T04:38:16.965Z
Learning: The pricing calculation logic should be shared between the web and mkt packages to ensure consistency.
🔇 Additional comments (4)
apps/mkt/components/pricing-cards.tsx (4)
1-9: LGTM! Well-structured imports and client directive.The component is properly set up with necessary imports and client-side rendering directive.
13-19: Extract pricing calculation logic to a shared utility.As previously discussed, the pricing calculation logic should be shared between the web and mkt packages to ensure consistency.
23-38: LGTM! Well-implemented accessible toggle.The billing frequency toggle is properly implemented with:
- Accessible Headless UI components
- Clear visual feedback
- Prominent display of the yearly discount
67-72: LGTM! Well-implemented contributor slider.The slider component is properly implemented with appropriate min/max values and value handling.
| <a | ||
| className="btn text-black font-semibold bg-green-400 border border-dark-400 w-full shadow" | ||
| href="https://github.com/apps/sweetr-dev/installations/new" | ||
| target="_blank" | ||
| rel="nofollow" | ||
| > |
There was a problem hiding this comment.
💡 Codebase verification
Security attributes missing or inconsistent across external links
The review comment is correct. External links with target="_blank" should include proper security attributes. This is a broader pattern in the codebase where security attributes are inconsistently applied:
- Some links use
rel="nofollow"only - Some use
rel="noreferrer"only - Some correctly use
rel="noopener noreferrer" - Some have no security attributes
Recommendation: Standardize all external links with target="_blank" to include rel="noopener noreferrer" for maximum security.
🔗 Analysis chain
Verify security attributes for external link.
The GitHub app installation link has target="_blank" but should also include rel="noopener" along with the existing rel="nofollow" to prevent potential security vulnerabilities.
- rel="nofollow"
+ rel="noopener nofollow"🏁 Scripts executed
The following scripts were executed for the analysis:
Error: No shell scripts found
Script:
#!/bin/bash
# Search for anchor tags with target="_blank"
rg --type tsx --type jsx 'target="_blank"' -B 1 -A 1
# Use ast-grep to find anchor elements with target="_blank"
ast-grep --pattern '<a $$$target="_blank"$$$>$$$</a>'
Length of output: 138
Script:
#!/bin/bash
# Search for target="_blank" in TypeScript/JavaScript files with surrounding context
rg 'target="_blank"' -g '*.{ts,tsx,js,jsx}' -B 2 -A 2
Length of output: 18238
waltergalvao
changed the title
waltergalvao
changed the title
Summary by CodeRabbit
Release Notes
New Features
Improvements
Dependencies
Styling