Pricing copy updates and smooth anchor links #495
Conversation
WalkthroughThe pull request introduces several changes across multiple files. The Changes
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (4)
src/core/Pricing/data.tsx (2)
95-95: Improved description for the Pro planThe change from "Scale with confidence." to "Scale business critical workloads." provides a more specific and impactful description of the Pro plan's capabilities. This aligns well with the PR objective of updating the pricing card copy.
Consider adding a hyphen to make it grammatically correct:
- content: "Scale business critical workloads.", + content: "Scale business-critical workloads.",
147-147: Improved description for the Enterprise planThe change from "Deliver without limits." to "Serious workloads without limits." provides a more impactful description of the Enterprise plan's capabilities. This aligns well with the PR objective of updating the pricing card copy and may resonate better with potential enterprise customers.
For consistency with the other plan descriptions, consider rephrasing slightly:
- content: "Serious workloads without limits.", + content: "Handle serious workloads without limits.",This change maintains the active voice used in other plan descriptions while keeping the emphasis on "serious workloads".
src/core/Pricing/PricingCards.tsx (2)
15-24: Smooth scrolling implementation looks good, but consider some improvements.The
onCtaClickfunction successfully implements smooth scrolling to a target element. However, there are a few points to consider:
The function doesn't handle the case where the element is not found. Consider adding an else clause or early return to handle this scenario.
The purpose of the 96-pixel offset is not clear. It would be helpful to add a comment explaining why this specific value is used (e.g., to account for a fixed header).
Using
document.getElementByIddirectly in React components can lead to issues with server-side rendering and may not always reflect the current state of the DOM. Consider using React refs or a more React-friendly approach to accessing DOM elements.Here's a suggested improvement:
const onCtaClick = (id: string) => { const targetId = id.split("#")[1]; const element = document.getElementById(targetId); if (element) { const HEADER_OFFSET = 96; // Adjust this value based on your header height const elementPosition = element.getBoundingClientRect().top + window.scrollY; window.scrollTo({ top: elementPosition - HEADER_OFFSET, behavior: "smooth", }); } else { console.warn(`Element with id "${targetId}" not found`); } };This version includes error handling, a named constant for the offset, and a more explicit calculation of the scroll position.
153-155: Smooth scrolling for CTA links implemented well, but consider accessibility and progressive enhancement.The changes successfully implement smooth scrolling for the CTA links using the new
onCtaClickfunction. However, there are a few points to consider:
For better accessibility, consider adding an
aria-labelto the link that describes its action. For example:aria-label={`Scroll to ${cta.text} section`}The current implementation relies on JavaScript for the scrolling behavior. For users with JavaScript disabled, the link will not work as expected. Consider using a progressive enhancement approach:
url={cta.url} onClick={(e) => { e.preventDefault(); onCtaClick(cta.url); }}This way, if JavaScript is disabled, the link will still function as a normal anchor link.
Here's a suggested improvement:
<FeaturedLink url={cta.url} additionalCSS={`sm:hidden group-hover:block font-medium ui-text-p3 ${t("text-neutral-500")} hover:${t("text-neutral-000")} transition-colors cursor-pointer`} onClick={(e) => { e.preventDefault(); onCtaClick(cta.url); }} aria-label={`Scroll to ${cta.text} section`} > {cta.text} </FeaturedLink>This version maintains the smooth scrolling behavior while improving accessibility and providing a fallback for users with JavaScript disabled.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (1)
src/core/Pricing/__snapshots__/PricingCards.stories.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (4)
- package.json (1 hunks)
- src/core/FeaturedLink.tsx (1 hunks)
- src/core/Pricing/PricingCards.tsx (2 hunks)
- src/core/Pricing/data.tsx (9 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🔇 Additional comments not posted (7)
src/core/FeaturedLink.tsx (1)
54-54: Approve with suggestions: Enhance accessibility and clarify implementationThe change adds flexibility to the
FeaturedLinkcomponent, allowing it to function as either a link or a button based on the presence of theonClickprop. This aligns with the PR objective of implementing "smooth anchor links" for feature CTAs.However, there are some concerns and suggestions:
Accessibility: When
onClickis provided andhrefis omitted, the component may look like a link but not behave like one, potentially causing accessibility issues.Developer expectations: The dual behavior might be confusing for developers who expect
FeaturedLinkto always be a link.Implementation clarity: It's not immediately clear how this change implements the "smooth anchor links" mentioned in the PR objectives.
To address these concerns:
- Add appropriate ARIA attributes to maintain accessibility:
<a - {...(onClick ? {} : { href: url })} + {...(onClick ? { role: "button", "aria-label": children?.toString() } : { href: url })} // ... other props >
Update the component's documentation to clearly explain its dual behavior.
Please provide more context on how this change implements smooth anchor links. Is there additional code in the
onClickhandler that manages smooth scrolling?To ensure this change doesn't break existing usage, let's check for any direct uses of the
hrefattribute onFeaturedLink:src/core/Pricing/data.tsx (6)
17-17: LGTM: Improved CTA URL for the Free planThe change from a placeholder "#" to a specific sign-up URL "https://ably.com/sign-up" is a good improvement. It provides a clear path for users to start with the free plan, enhancing the user experience.
64-64: LGTM: Improved CTA URL for the Standard planThe change from "#" to "https://ably.com/users/paid_sign_up" for the Standard plan's CTA is a good improvement. It provides a direct link for users to sign up for a paid plan, enhancing the user experience and potentially improving conversion rates.
83-83: LGTM: Consistent anchor link implementationThe change from "#" to "#pricing-table" for the "See all features" CTA in the Standard plan is consistent with the similar change made for the Free plan. This maintains a uniform navigation experience across different plan types.
Line range hint
1-281: Summary of changesThe updates to
src/core/Pricing/data.tsxalign well with the PR objectives:
- CTA URLs have been updated to provide clear paths for user actions.
- Anchor links have been implemented for improved navigation within the page.
- Descriptions for the Pro and Enterprise plans have been refined for better clarity and impact.
These changes should enhance the user experience and potentially improve conversion rates. The implementation is consistent across different plan types, maintaining a uniform experience.
A few minor suggestions have been made for further improvements:
- Adding a hyphen in "business-critical" for grammatical correctness.
- Slight rephrasing of the Enterprise plan description for consistency.
Please ensure that all anchor links are correctly implemented and that smooth scrolling functionality is working as expected across the page.
115-115: LGTM: Improved CTAs for Pro and Enterprise plans
The change of the Pro plan CTA URL to "https://ably.com/users/paid_sign_up" is consistent with the Standard plan, providing a clear path for users to sign up.
The updates to the Enterprise plan CTA are good improvements:
- The new URL "#pricing-enterprise" allows for easy navigation to the enterprise pricing section.
- The addition of styling classes enhances the visual appeal of the CTA button.
These changes align well with the PR objectives of updating CTA URLs and enhancing clarity.
Please ensure that:
- The "#pricing-enterprise" anchor exists on the page.
- The smooth scrolling functionality works correctly for this new anchor.
You can verify this by running the following script:
#!/bin/bash # Check if the pricing-enterprise anchor exists in the HTML files rg --type html 'id="pricing-enterprise"' # Check if smooth scrolling is implemented rg --type js 'scroll-behavior:\s*smooth'Also applies to: 152-156
32-32: LGTM: Improved navigation with anchor linkThe change from "#" to "#pricing-table" for the "See all features" CTA is a good improvement. It allows users to navigate directly to the pricing table section of the page.
Please ensure that:
- The "#pricing-table" anchor exists on the page.
- The smooth scrolling functionality is implemented and working correctly.
You can verify this by running the following script:
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
2f40bc2 to
56b3176
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (4)
src/core/Pricing/types.ts (1)
9-15: LGTM! Consider adding JSDoc comments for clarity.The additions to
PricingDataFeatureCtatype align well with the PR objectives. The newdisabledandonClickproperties provide more control over CTA behavior, supporting the implementation of smooth anchor links and improved navigation.Consider adding JSDoc comments to explain the purpose of each property, especially the new ones. This would enhance code readability and maintainability. For example:
type PricingDataFeatureCta = { /** The text to display on the CTA button */ text: string; /** The URL to navigate to when the CTA is clicked */ url: string; /** Optional CSS class name for custom styling */ className?: string; /** Whether the CTA is disabled */ disabled?: boolean; /** Custom click handler for the CTA */ onClick?: () => void; };src/core/Pricing/PricingCards.stories.tsx (1)
45-47: New exportConsumptionNoDelimiterlooks good, but consider adding a comment for clarity.The new export
ConsumptionNoDelimiteris correctly implemented and follows the established pattern in the file. It provides an additional rendering option for theTemplatecomponent without a delimiter, which aligns with the component's flexibility described in the documentation.Consider adding a brief comment above this export to explain its purpose, similar to how you might document it in the Storybook UI. This would improve code readability and maintainability. For example:
+// Renders the consumption pricing cards without a delimiter between columns export const ConsumptionNoDelimiter = { render: () => <Template data={consumptionData} delimiter="blank" />, };src/core/Pricing/PricingCards.tsx (2)
15-23: Smooth scrolling implementation looks good, with room for improvementThe
onCtaClickfunction successfully implements smooth scrolling to anchor links, which aligns with the PR objectives. However, consider the following improvements:
- The 96px offset is hardcoded. Consider making this a configurable prop to accommodate potential changes in header height.
- Add error handling for cases where the element with the given id is not found.
Here's a suggested improvement:
const onCtaClick = (id: string, offset = 96) => { const element = document.getElementById(id.split("#")[1]); if (element) { window.scrollTo({ top: element.getBoundingClientRect().top + window.scrollY - offset, behavior: "smooth", }); } else { console.warn(`Element with id ${id} not found`); } };
158-160: LGTM: Implemented smooth scrolling for section CTAsThe changes to the
FeaturedLinkcomponent within the pricing card sections successfully implement the smooth scrolling behavior:
- Setting
url="#"is consistent with the new smooth scrolling implementation.- Adding
cursor-pointertoadditionalCSSimproves the user experience.- Using
onCtaClickin theonClickprop implements the smooth scrolling functionality.These changes align perfectly with the PR objectives.
Consider extracting the
additionalCSSstring into a constant or a function to improve readability and maintainability, especially if this styling is used in multiple places.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
🔇 Files ignored due to path filters (2)
src/core/FeaturedLink/__snapshots__/FeaturedLink.stories.tsx.snapis excluded by!**/*.snapsrc/core/Pricing/__snapshots__/PricingCards.stories.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (6)
- src/core/FeaturedLink.tsx (4 hunks)
- src/core/FeaturedLink/FeaturedLink.stories.tsx (1 hunks)
- src/core/Pricing/PricingCards.stories.tsx (1 hunks)
- src/core/Pricing/PricingCards.tsx (4 hunks)
- src/core/Pricing/data.tsx (13 hunks)
- src/core/Pricing/types.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/FeaturedLink.tsx
- src/core/Pricing/data.tsx
🔇 Additional comments not posted (8)
src/core/FeaturedLink/FeaturedLink.stories.tsx (1)
45-49: LGTM! Good addition of a Disabled state story.The addition of a Disabled state story for the FeaturedLink component is a valuable inclusion. It allows for better testing and visualization of the component in various states, which is crucial for maintaining a robust component library.
However, I noticed a slight discrepancy between the PR objectives and this specific change. The PR summary mentions "Pricing copy updates and smooth anchor links", but this file is modifying a FeaturedLink component story. Could you clarify if this change is related to the PR objectives or if it's an additional improvement?
To verify the usage of the
disabledprop in the FeaturedLink component, let's run the following script:src/core/Pricing/types.ts (1)
20-20: Verify intention ofamounttype change and consider alternative.The change from
number | stringtostringfor theamountproperty inPricingDataFeaturetype is a good step towards consistency. However, it's worth verifying if this change was intentional, as it wasn't explicitly mentioned in the PR objectives.Please confirm that changing
amountto be strictly a string type was intentional. This change could potentially break existing code that uses numbers for the amount.If you want to maintain flexibility while ensuring proper formatting, consider using a branded type:
type Currency = string & { __brand: 'Currency' }; type PricingDataFeature = { // ... price: { amount: Currency; content?: ReactNode }; // ... }; // Usage const formatCurrency = (value: number | string): Currency => { // Implement currency formatting logic here return `$${value}` as Currency; };This approach ensures that
amountis always a string while allowing both numbers and strings to be used when creating aPricingDataFeature, as long as they're passed through theformatCurrencyfunction.To verify the impact of this change, you can run the following script:
This script will help identify any potential areas in the codebase where the
amountfield might still be using a number type.✅ Verification successful
Verified the
amounttype change inPricingDataFeature.No instances of
PricingDataFeatureusage withamountas a number were found, confirming that changingamountto a strictlystringtype is intentional and does not break existing code.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find usages of PricingDataFeature where amount might be a number # Test: Search for PricingDataFeature usage with number literals in the amount field echo "Searching for PricingDataFeature usage with number literals in the amount field:" rg --type typescript 'PricingDataFeature.*amount:\s*\d+' -g '!**/types.ts' # Test: Search for variable assignments to PricingDataFeature's amount field echo "Searching for variable assignments to PricingDataFeature's amount field:" rg --type typescript 'amount:\s*\w+' -g '!**/types.ts'Length of output: 532
Script:
#!/bin/bash # Description: Find usages of PricingDataFeature where amount might be a number # Test: Search for PricingDataFeature usage with number literals in the amount field echo "Searching for PricingDataFeature usage with number literals in the amount field:" rg 'PricingDataFeature.*amount:\s*\d+' -g '*.ts' -g '*.tsx' # Test: Search for variable assignments to PricingDataFeature's amount field echo "Searching for variable assignments to PricingDataFeature's amount field:" rg 'amount:\s*\w+' -g '*.ts' -g '*.tsx'Length of output: 508
src/core/Pricing/PricingCards.stories.tsx (2)
Line range hint
1-47: Summary: New export adds flexibility to PricingCards storiesThe addition of the
ConsumptionNoDelimiterexport enhances the flexibility of the PricingCards stories by providing an option to render the consumption pricing cards without a delimiter. This change is consistent with the existing code structure and patterns.Key points:
- The new export follows the established pattern in the file.
- It introduces a new use case for the
delimiterprop with the "blank" value.- The change is minimal and focused, not affecting existing functionality.
To ensure full compatibility:
- Verify the handling of the "blank" delimiter in the PricingCards component (as mentioned in the previous comment).
- Consider adding a brief comment to explain the purpose of this new export.
- Update the component's documentation if necessary to reflect this new usage option.
Overall, this change appears to be a valuable addition to the PricingCards stories, providing more flexibility in how the component can be displayed and tested.
45-47: Verify handling of "blank" delimiter in PricingCards componentThe new
ConsumptionNoDelimiterexport introduces a "blank" delimiter, which is a new usage of thedelimiterprop. To ensure this change is fully supported:
- Verify that the PricingCards component correctly handles the "blank" delimiter value.
- Update the component's documentation to include this new option if it's not already mentioned.
Please run the following script to check the PricingCards component implementation:
This script will help us confirm that the "blank" delimiter is properly handled and documented in the PricingCards component.
src/core/Pricing/PricingCards.tsx (4)
12-13: LGTM: Expanded delimiter prop typeThe addition of
"blank"as a possible value for thedelimiterprop provides more flexibility in the component's usage without breaking existing implementations. This change is well-aligned with the subsequent implementation updates.
36-45: LGTM: Proper implementation of "blank" delimiter optionThe changes to the
delimiterColumnfunction correctly implement the new "blank" delimiter option. The conditional rendering and margin adjustment are appropriate and consistent with the earlier type definition change.
96-99: LGTM: Improved UX for FeaturedLinkThe changes to the
FeaturedLinkcomponent enhance the user experience:
- Adding
cursor-pointervisually indicates that the link is clickable.- Passing through the
onClickprop allows for custom click handling, which is consistent with the new smooth scrolling functionality.These changes align well with the PR objectives.
Line range hint
1-180: Overall assessment: Well-implemented features with minor improvement opportunitiesThe changes in this file successfully implement the smooth scrolling functionality and the "blank" delimiter option for pricing cards. The code is consistent, aligns well with the PR objectives, and maintains good quality. The suggested minor improvements, if implemented, would further enhance the code's robustness and maintainability.
Great job on these updates!
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
4154bc6 to
6b63e1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/core/Pricing/types.ts (2)
9-15: LGTM! Consider adding JSDoc comments for better documentation.The additions to
PricingDataFeatureCtatype enhance its flexibility without breaking existing implementations. Thedisabledproperty allows for conditional rendering or styling, whileonClickenables custom click handling.Consider adding JSDoc comments to describe the purpose of each property, especially the new ones. For example:
type PricingDataFeatureCta = { /** The text to display on the CTA button */ text: string; /** The URL to navigate to when the CTA is clicked */ url: string; /** Optional CSS class name for custom styling */ className?: string; /** Whether the CTA should be disabled */ disabled?: boolean; /** Custom click handler for the CTA */ onClick?: () => void; };
17-22: LGTM! Consider adding a type alias foritems.The introduction of
PricingDataFeatureSectiontype improves code organization and type safety by providing a clear structure for feature sections. It encapsulates all necessary properties and allows for flexible content structure in theitemsproperty.Consider creating a type alias for the
itemsproperty to improve readability:type FeatureItems = string[] | string[][]; export type PricingDataFeatureSection = { title: string; items: FeatureItems; listItemColors?: { foreground: ColorClass; background: ColorClass }; cta?: PricingDataFeatureCta; };This change would make the type definition more self-documenting and easier to maintain if the structure of
itemsneeds to be modified in the future.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
src/core/Pricing/__snapshots__/PricingCards.stories.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (3)
- src/core/Pricing/PricingCards.tsx (4 hunks)
- src/core/Pricing/data.tsx (13 hunks)
- src/core/Pricing/types.ts (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/Pricing/PricingCards.tsx
- src/core/Pricing/data.tsx
🔇 Additional comments (1)
src/core/Pricing/types.ts (1)
27-27: Approvesectionschange, but verifyprice.amounttype change.The use of
PricingDataFeatureSectionfor thesectionsproperty improves code organization and type safety. However, changingprice.amountfromnumber | stringto juststringmight break existing code that relies on numeric values.Please verify that all usages of
price.amountin the codebase are compatible with thestringtype. Run the following script to find all occurrences:If any numeric usages are found, consider keeping the type as
number | stringor update all occurrences to use string values.Also applies to: 29-29
✅ Verification successful
Approve both
sectionsandprice.amounttype changes.The use of
PricingDataFeatureSectionfor thesectionsproperty enhances code organization and type safety. Additionally, the change ofprice.amounttostringdoes not appear to affect any existing code that relies on numeric values.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Find all usages of price.amount in the codebase # Test: Search for price.amount usages rg --type typescript -A 5 'price\.amount'Length of output: 78
Script:
#!/bin/bash # Description: Find all usages of price.amount in the codebase # Test: Search for price.amount usages in TypeScript files rg --type ts -A 5 'price\.amount'Length of output: 466
jamiehenson
force-pushed
the
more-pricing-stuff
branch
from
b8445a8 to
8222a6c
Compare
Summary of changes
Updates copy for the pricing cards and introduces the means to scroll smoothly to somewhere else in the page in the feature CTAs.
How do you manually test this?
Reviewer Tasks (optional)
Merge/Deploy Checklist
Frontend Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Chores