-
-
Notifications
You must be signed in to change notification settings - Fork 103
Add sponsor section component #1536
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
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughA sponsor section was added to the Changes
Assessment against linked issues
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
Documentation and Community
|
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
frontend/src/types/card.ts (1)
43-43
: Consider renaming the property to avoid confusion with React's key prop.While this won't cause technical issues since the prop is destructured in the component, using
key
as a property name could be confusing since React useskey
as a special prop for reconciliation. Consider a more descriptive name likeentityKey
oridentifier
.- key?: string + entityKey?: string
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
frontend/src/components/CardDetailsPage.tsx
(4 hunks)frontend/src/types/card.ts
(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
frontend/src/components/CardDetailsPage.tsx (3)
backend/apps/owasp/graphql/nodes/chapter.py (1)
key
(49-51)backend/apps/owasp/graphql/nodes/project.py (1)
key
(43-45)backend/apps/github/models/common.py (1)
title
(35-39)
🔇 Additional comments (5)
frontend/src/components/CardDetailsPage.tsx (5)
12-12
: Good addition of Next.js Link import.The Link import is necessary for the new sponsor section functionality and follows Next.js best practices.
39-39
: Parameter addition aligns with interface change.The
key
parameter correctly matches the new optional property in DetailsCardProps interface.
52-66
: Robust URL construction with proper encoding.The function correctly handles different entity types and safely encodes the title parameter. The fallback URL ensures functionality even when the key is missing.
The logic appropriately:
- Provides a fallback donation URL when no key is provided
- Uses different repository prefixes based on entity type
- Safely encodes the title to prevent URL issues
- Constructs query parameters correctly
68-68
: Clear conditional logic for sponsor display.The boolean clearly defines which entity types should show the sponsor section, matching the PR requirements.
223-241
: Well-implemented sponsor section with good accessibility.The sponsor section implementation includes:
- Proper conditional rendering
- Accessibility attributes (
target="_blank"
,rel="noopener noreferrer"
)- Responsive styling with dark mode support
- Dynamic button text based on entity type
- Appropriate positioning at the bottom of the page
The UI provides a clear call-to-action while maintaining consistency with the existing design system.
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.
Thanks for working on this. You'll need to fix some issues:
@@ -40,6 +40,7 @@ export interface DetailsCardProps { | |||
geolocationData?: GeoLocDataGraphQL | null | |||
heatmap?: JSX.Element | |||
is_active?: boolean | |||
key?: string |
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.
@M-ayank2005 You added this new property here, but I don't see any key being passed on any page 🤔
Without it - links don't work as they should.
Could you also add tests to cover this new component added to project, chapter and repo pages? |
@@ -51,19 +51,16 @@ const DetailsCard = ({ | |||
}: DetailsCardProps) => { | |||
const getDonationUrl = () => { | |||
if (!key) return 'https://owasp.org/donate/' |
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.
This is still not addressed. No key is present (ever) so all links only direct to https://owasp.org/donate/
You need to update the backend to pass in this data for a key.
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.
@kasya Will it work as expected just by passing key in DetailsCard props?
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.
@M-ayank2005 depends on what you expect to have in that key
🤔 . But I assume it should.
The thing is - you need to actually work on passing it in in this same PR.
|
@M-ayank2005 Hi! Any updates here? |
This PR adds a contextual sponsor section to the details pages for certain entity types.
Resolves: #1424
Changes Made :
Sponsor section UI:
Placement: The sponsor section appears at the bottom of the relevant details pages.