fix: rendering issue with few components using next-i18n on community pages #3384
Conversation
…n community/tsc , community/events , community pages
SahilDahekar
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThe pull request introduces a new React component, Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
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 (
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-3384--asyncapi-website.netlify.app/ |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3384 +/- ##
=======================================
Coverage 67.77% 67.77%
=======================================
Files 21 21
Lines 664 664
=======================================
Hits 450 450
Misses 214 214 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (4)
utils/getStatic.ts (2)
32-32: LGTM! Consider improving type safety.The addition of the English locale fallback is a good defensive programming practice. However, the function could benefit from stronger typing.
Consider this type-safe improvement:
- export async function getI18nProps(ctx: any, ns = ['common']) { - const locale = ctx?.params?.lang ? ctx?.params?.lang : 'en'; + type LocaleContext = { + params?: { + lang?: string; + }; + }; + export async function getI18nProps(ctx: LocaleContext, ns = ['common']) { + const locale = ctx?.params?.lang || 'en';
32-32: Simplify the conditional expression.The current implementation uses a verbose ternary expression with multiple optional chaining operators.
Consider this more concise version:
- const locale = ctx?.params?.lang ? ctx?.params?.lang : 'en'; + const locale = ctx?.params?.lang || 'en';pages/community/events/index.tsx (1)
Line range hint
35-44: Consider enhancing calendar button accessibilityThe calendar integration buttons could benefit from additional accessibility attributes.
Consider adding ARIA labels to provide more context:
<GoogleCalendarButton + aria-label="Add AsyncAPI events to Google Calendar" href='https://calendar.google.com/calendar/u/3?cid=Y19xOXRzZWlnbG9tZHNqNm5qdWh2YnB0czExY0Bncm91cC5jYWxlbmRhci5nb29nbGUuY29t' /> <ICSFileButton + aria-label="Download AsyncAPI events calendar file" href='https://calendar.google.com/calendar/ical/c_q9tseiglomdsj6njuhvbpts11c%40group.calendar.google.com/public/basic.ics' className='mt-2 md:ml-2 md:mt-0' />pages/community/tsc.tsx (1)
74-75: LGTM: Improved icon components with CSS-based hover effectsThe refactoring improves performance by replacing JavaScript state management with CSS hover effects. However, there's an opportunity to make the hover colors more consistent across icons.
Consider standardizing the hover colors for better visual consistency:
- <IconTwitter className={'hover:fill-black'} /> + <IconTwitter className={'hover:fill-primary-600'} /> - <IconGithub className={'hover:fill-black'} /> + <IconGithub className={'hover:fill-primary-600'} /> - <IconLinkedIn className={'hover:fill-linkedin'} /> + <IconLinkedIn className={'hover:fill-primary-600'} />Also applies to: 87-88, 100-102
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/CommunityEvents.tsx(1 hunks)pages/community/events/index.tsx(2 hunks)pages/community/index.tsx(2 hunks)pages/community/tsc.tsx(5 hunks)utils/getStatic.ts(1 hunks)
🔇 Additional comments (10)
components/CommunityEvents.tsx (3)
1-11: LGTM! Well-organized imports with proper type definitions.
The imports are logically grouped and include all necessary components and types for the functionality.
16-42: LGTM! Well-structured responsive layout with proper error handling.
The component implements:
- Responsive grid layout using Tailwind classes
- Proper error handling for empty states
- Clear visual hierarchy with appropriate spacing
1-45: Verify i18n integration with this component.
While the component has been successfully extracted, please ensure that:
- All text content is properly internationalized
- The component works correctly with the updated
getI18nProps()function
pages/community/events/index.tsx (3)
4-4: LGTM: Proper implementation of i18n static props
The implementation of getStaticProps using makeStaticProps with the necessary i18n namespaces ('landing-page', 'footer', 'common') aligns well with fixing the rendering issues mentioned in the PR objectives.
Also applies to: 7-7, 18-20
91-91: LGTM: Clean integration of CommunityEvents component
The extraction of events listing into a separate CommunityEvents component improves code organization and aligns with the PR objectives.
Line range hint 27-156: Verify performance impact of the component structure
The Lighthouse report shows a low performance score (46). Let's verify if the component structure could be optimized.
✅ Verification successful
Performance impact verified and no issues found in the component structure.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential performance optimizations in the component
# Look for large component renders without memo
ast-grep --pattern 'export default function $COMPONENT() {
$$$
return (
$$$
<div className=$_>
$$$
</div>
$$$
);
}'
# Check for missing image optimizations
rg -l "img.*src=.*\.(png|jpg|jpeg|webp)"
# Look for potential code splitting opportunities
ast-grep --pattern 'import $COMPONENT from $PATH'
Length of output: 976
pages/community/index.tsx (2)
6-6: LGTM!
The import of makeStaticProps is correctly added and follows project conventions.
18-20: Implementation looks good but verify i18n integration.
The addition of getStaticProps with the 'common' namespace should resolve the rendering issues with next-i18n. This ensures translations are available during static site generation.
Let's verify the i18n integration:
✅ Verification successful
i18n integration verified and functioning correctly.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify i18n setup and usage
# Check for translation files and their usage
# Check if common namespace translations exist
echo "Checking for common namespace translations..."
fd -e json . public/locales -x echo "Found translation file: {}"
# Check for other components using makeStaticProps with common namespace
echo "\nChecking for similar implementations..."
rg "makeStaticProps\(\['common'\]\)" -A 2 -B 2
# Verify NewsletterSubscribe component usage
echo "\nChecking NewsletterSubscribe usage..."
rg "NewsletterSubscribe" -A 2 -B 2
Length of output: 9310
pages/community/tsc.tsx (2)
4-4: LGTM: Proper implementation of getStaticProps for i18n support
The implementation correctly uses makeStaticProps utility with the 'common' namespace, which aligns with Next.js internationalization best practices.
Also applies to: 14-16
Line range hint 319-324: Verify NewsletterSubscribe component's button labels
Given issue #3381 about incorrect button names, we should verify the labels rendered by the NewsletterSubscribe component.
| {events.map((event: Event, index: number) => { | ||
| return <EventPostItem key={index} id={event.title} post={event} />; | ||
| })} |
There was a problem hiding this comment.
Avoid using array indices as React keys.
Using array indices as keys can lead to issues with component state and performance when the list items can change (reordering, filtering, etc.). Consider using a unique identifier from the event data.
-{events.map((event: Event, index: number) => {
- return <EventPostItem key={index} id={event.title} post={event} />;
+{events.map((event: Event) => {
+ return <EventPostItem key={event.title} id={event.title} post={event} />;Committable suggestion skipped: line range outside the PR's diff.
| const CommunityEvents = () => { | ||
| const [events, setEvents] = useState(getEvents(meetings)); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize state initialization to prevent unnecessary calculations.
The getEvents(meetings) call in useState will run on every render. Consider optimizing this by either:
- Moving the initialization outside the component
- Using useMemo to memoize the result
+const initialEvents = getEvents(meetings);
+
const CommunityEvents = () => {
- const [events, setEvents] = useState(getEvents(meetings));
+ const [events, setEvents] = useState(initialEvents);Or with useMemo:
const CommunityEvents = () => {
- const [events, setEvents] = useState(getEvents(meetings));
+ const initialEvents = useMemo(() => getEvents(meetings), []);
+ const [events, setEvents] = useState(initialEvents);Committable suggestion skipped: line range outside the PR's diff.
|
Potential duplicate of #3308 |
|
Should this PR be closed then since this is a duplicate ?? @bandantonio |
|
@SahilDahekar I think yes. You can always compare the changes in both PRs and decide for yourself. But since the issue has already been closed, the linked PR can also be closed. If you find that the original PR lacks some info or can be improved, feel free to contribute to that PR directly but please try to avoid the situation, that was mentioned in that PR
|
|
Thanks @bandantonio for clarification . I am closing this PR and will directly contribute to the #3308 . |
Description
NewsletterSubscribecomponent rendering issue on/community/tsc,/community/events,/communitypage by addinggetStaticProps()function./community/eventspage extractedAll Eventssection into new componentCommunityEventsto be able to usegetStaticProps()function.useStatehook use for icon hover functionality on/community/tscpage.getI18nProps()function in thegetStatic.tsby adding default locale asen.Related issue(s)
Fixes #3381
Summary by CodeRabbit
New Features
CommunityEventscomponent for displaying and filtering community events.Bug Fixes
Refactor