feat: enabling i18n feature for smaller screens #3556
Conversation
devilkiller-ag
requested review from
derberg,
akshatnema,
magicmatatjahu,
anshgoyalevil,
Mayaleeeee and
asyncapi-bot-eve
as code owners
WalkthroughThis pull request introduces a comprehensive enhancement to the internationalization (i18n) feature for the AsyncAPI website, focusing on enabling language selection across different screen sizes. The changes include adding a new Changes
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
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
Documentation and Community
|
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #3556 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 20 20
Lines 732 732
=========================================
Hits 732 732 ☔ 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: 1
🔭 Outside diff range comments (1)
utils/getStatic.ts (1)
Line range hint
32-36: Improve type safety and error handlingThe function uses loose typing and could benefit from better error handling.
-export async function getI18nProps(ctx: any, ns = ['common']) { +interface I18nContext { + params?: { + lang?: string; + }; +} + +export async function getI18nProps(ctx: I18nContext, ns = ['common']) { const locale = ctx?.params?.lang ? ctx.params.lang : 'english'; + + if (!i18nextConfig.i18n.locales.includes(locale)) { + console.warn(`Invalid locale: ${locale}, falling back to english`); + return serverSideTranslations('english', ns); + } + const props = { ...(await serverSideTranslations(locale, ns)) };
🧹 Nitpick comments (7)
next-i18next.config.js (2)
3-4: Consider using standard ISO 639-1 language codesUsing full language names ('english', 'deutsch') instead of standard ISO 639-1 codes ('en', 'de') deviates from web standards. This could impact:
- SEO optimization
- Browser language detection
- Accessibility tools
- Integration with third-party i18n tools
Consider keeping ISO codes internally while displaying full names in the UI.
🧰 Tools
🪛 eslint
[error] 3-3: Delete
··(prettier/prettier)
[error] 4-4: Replace
······defaultLocale·with····defaultLocale(prettier/prettier)
3-9: Fix formatting issuesThere are inconsistent indentation and spacing issues in the configuration.
module.exports = { i18n: { - locales: ['english', 'deutsch'], - defaultLocale : 'english', - namespaces: ['landing-page', 'common', 'tools'], - defaultNamespace: 'landing-page', - react: { useSuspense: false },// this line - }, + locales: ['english', 'deutsch'], + defaultLocale: 'english', + namespaces: ['landing-page', 'common', 'tools'], + defaultNamespace: 'landing-page', + react: { useSuspense: false } + } };🧰 Tools
🪛 eslint
[error] 3-3: Delete
··(prettier/prettier)
[error] 4-4: Replace
······defaultLocale·with····defaultLocale(prettier/prettier)
[error] 5-5: Delete
··(prettier/prettier)
[error] 6-6: Delete
··(prettier/prettier)
[error] 7-7: Replace
··react:·{·useSuspense:·false·},withreact:·{·useSuspense:·false·}·(prettier/prettier)
[error] 8-8: Replace
··},with}(prettier/prettier)
utils/i18n.ts (1)
20-21: Enhance type safety for language identifiersConsider using TypeScript enums or const assertions for better type safety and autocompletion.
-export const languages = ['english', 'deutsch']; -export const defaultLanguage = 'english'; +export const languages = ['english', 'deutsch'] as const; +export type SupportedLanguage = typeof languages[number]; +export const defaultLanguage: SupportedLanguage = 'english';components/icons/Language.tsx (2)
4-6: Enhance component documentationThe JSDoc comment could be more descriptive about the component's purpose and usage.
/** - * @description Icons for asyncapi website + * @description Language icon component used in the language selector + * @param {Object} props - Component props + * @param {string} [props.className=''] - Additional CSS classes to apply + * @returns {JSX.Element} Language icon SVG */
7-23: Add accessibility attributes to SVGThe SVG icon should include ARIA attributes for better accessibility.
-export default function IconLanguage({ className = '' }) { +export default function IconLanguage({ className = '' }: { className?: string }) { return ( <svg xmlns='http://www.w3.org/2000/svg' fill='none' viewBox='0 0 24 24' strokeWidth={1.5} stroke='currentColor' + role="img" + aria-label="Language selector" className={`size-5 ${className}`} >components/languageSelector/LanguageSelect.tsx (1)
16-35: LGTM! Well-structured language selector implementation.The changes effectively integrate the language icon while maintaining accessibility and proper spacing. The use of relative positioning and flex layout ensures proper alignment.
A minor suggestion to enhance accessibility:
- <select + <select + aria-label="Select language" data-testid='Select-form'components/navigation/NavBar.tsx (1)
236-236: Reduce duplication in language name formatting.The language name capitalization logic is repeated in multiple places. Consider extracting it to a utility function.
+ // Add to utils/i18n.ts + export const formatLanguageName = (lang: string): string => + lang ? lang.charAt(0).toUpperCase() + lang.slice(1) : 'English'; - selected={i18n.language ? i18n.language.charAt(0).toUpperCase() + i18n.language.slice(1) : 'English'} + selected={formatLanguageName(i18n.language)} - currentLanguage={i18n.language ? i18n.language.charAt(0).toUpperCase() + i18n.language.slice(1) : 'English'} + currentLanguage={formatLanguageName(i18n.language)}Also applies to: 254-254
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
components/icons/Icons.mdx(2 hunks)components/icons/Language.tsx(1 hunks)components/languageSelector/LanguageSelect.tsx(2 hunks)components/navigation/MobileNavMenu.tsx(4 hunks)components/navigation/NavBar.tsx(5 hunks)next-i18next.config.js(1 hunks)utils/getStatic.ts(1 hunks)utils/i18n.ts(1 hunks)
🧰 Additional context used
🪛 eslint
next-i18next.config.js
[error] 3-3: Delete ··
(prettier/prettier)
[error] 4-4: Replace ······defaultLocale· with ····defaultLocale
(prettier/prettier)
[error] 5-5: Delete ··
(prettier/prettier)
[error] 6-6: Delete ··
(prettier/prettier)
[error] 7-7: Replace ··react:·{·useSuspense:·false·}, with react:·{·useSuspense:·false·}·
(prettier/prettier)
[error] 8-8: Replace ··}, with }
(prettier/prettier)
🔇 Additional comments (4)
utils/i18n.ts (1)
8-17: Validate path consistencyThe i18nPaths contain relative URLs. Consider:
- Validating paths against your router configuration
- Using path constants to prevent typos
- Adding comments for path purpose
components/navigation/MobileNavMenu.tsx (1)
23-25: LGTM! Well-typed props interface.The props interface clearly defines the required types for language handling.
components/navigation/NavBar.tsx (1)
Line range hint
46-62: LGTM! Clear documentation and implementation of getUniqueLangs.The function properly handles language name capitalization and provides a sensible default.
components/icons/Icons.mdx (1)
33-33: LGTM! Proper integration of language icon.The IconLanguage component is correctly imported and documented in the icon gallery, following the established pattern.
Also applies to: 240-242
| <div className='space-y-2 px-5 py-2' onClick={() => showMenu('language')}> | ||
| <div className='grid gap-4'> | ||
| <div> | ||
| <h4 className='mb-4 flex justify-between font-medium text-gray-800'> | ||
| <a className='flex cursor-pointer items-center gap-x-2'> | ||
| Language <IconLanguage /> | ||
| </a> | ||
| <NavItemDropdown /> | ||
| </h4> | ||
| {open === 'language' && | ||
| uniqueLangs.map((lang) => ( | ||
| <button | ||
| key={lang.key} | ||
| onClick={() => changeLanguage(lang.value.toLowerCase(), true)} | ||
| className={`mb-4 ml-2 block w-full rounded-lg py-1 text-start text-sm font-medium leading-6 text-gray-700 transition duration-150 ease-in-out hover:bg-gray-50 ${currentLanguage.toLowerCase() === lang.text.toLowerCase() ? 'text-secondary-500' : ''}`} | ||
| data-testid='MobileNav-language-item' | ||
| > | ||
| {lang.text} | ||
| </button> | ||
| ))} | ||
| </div> | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve accessibility of the language selection menu.
The current implementation has two potential accessibility issues:
- Using a div with onClick instead of a button for the language menu trigger
- Missing ARIA attributes for the expandable menu
Consider this improvement:
- <div className='space-y-2 px-5 py-2' onClick={() => showMenu('language')}>
+ <div className='space-y-2 px-5 py-2'>
+ <button
+ onClick={() => showMenu('language')}
+ aria-expanded={open === 'language'}
+ aria-controls="language-menu"
+ className="w-full text-left"
+ >
<div className='grid gap-4'>
<div>
<h4 className='mb-4 flex justify-between font-medium text-gray-800'>
<a className='flex cursor-pointer items-center gap-x-2'>
Language <IconLanguage />
</a>
<NavItemDropdown />
</h4>
- {open === 'language' &&
+ <div
+ id="language-menu"
+ role="menu"
+ className={open === 'language' ? 'block' : 'hidden'}
+ >
uniqueLangs.map((lang) => (
<button
key={lang.key}
onClick={() => changeLanguage(lang.value.toLowerCase(), true)}
+ role="menuitem"
className={`mb-4 ml-2 block w-full rounded-lg py-1 text-start text-sm font-medium leading-6 text-gray-700 transition duration-150 ease-in-out hover:bg-gray-50 ${
currentLanguage.toLowerCase() === lang.text.toLowerCase() ? 'text-secondary-500' : ''
}`}
data-testid='MobileNav-language-item'
>
{lang.text}
</button>
- ))}
+ )}
+ </div>
</div>
</div>
+ </button>
</div>Committable suggestion skipped: line range outside the PR's diff.
asyncapi-bot
requested review from
BhaswatiRoy,
J0SAL,
TRohit20 and
VaishnaviNandakumar
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
ADDING_TRANSLATIONS.md (4)
Line range hint
21-30: Update directory structure examples for consistencyThe directory structure examples show the correct full language names (
deutsch,english), but there's an inconsistency in the ellipsis notation. The JSON files list ends with both┗ 📜tools.jsonand┗ 📜....json.📦locales ┣ 📂deutsch ┃ ┣ 📜common.json ┃ ┣ 📜landing-page.json - ┃ ┗ 📜tools.json - ┃ ┗ 📜....json + ┃ ┣ 📜tools.json + ┃ ┗ 📜....json🧰 Tools
🪛 Markdownlint (0.37.0)
19-19: null
Fenced code blocks should have a language specified(MD040, fenced-code-language)
134-135: Improve clarity in routing configuration instructionsThe instructions for configuring i18n routing could be clearer:
- The phrase "exact same" is redundant
- The example immediately jumps to newsletter page without proper context
- - Make sure to add the exact same `href` to the `utils/i18n.ts` in the respective locales which support that `href`. + - Add the corresponding `href` to the `utils/i18n.ts` file for each supported locale. - For example, if you want to translate the `pages/newsletter.tsx` page, so that if someone visits `asyncapi.com/deutsch/newsletter`, it shows the page in the `German` locale. + For example, to translate the `pages/newsletter.tsx` page and make it accessible at `asyncapi.com/deutsch/newsletter` in German:Also applies to: 137-139
🧰 Tools
🪛 LanguageTool
[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ng referenced. - Make sure to add the exact samehrefto theutils/i18n.tsin the re...(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 Markdownlint (0.37.0)
134-134: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
135-135: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
244-246: Fix grammatical error in locale descriptionThere's a grammatical error in the description of existing locales.
-There exist a few locales like `english` (English) and `deutsch` (German) which have available localizations present. +There exists a few locales like `english` (English) and `deutsch` (German) which have available localizations.🧰 Tools
🪛 LanguageTool
[grammar] ~244-~244: Did you mean “exists”?
Context: ... people from all over the world. There exist a few locales likeenglish(English) ...(THERE_VBP_A_NN)
134-135: Fix Markdown formatting inconsistenciesThe document contains inconsistent list indentation levels. According to Markdownlint:
- Unordered lists should have consistent indentation (0 spaces expected, 2 spaces found)
- Some sections use emphasis (
**Note**) where headings would be more appropriateConsider using a Markdown linter to automatically fix these formatting issues.
Also applies to: 249-252, 255-256, 260-260
🧰 Tools
🪛 LanguageTool
[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ng referenced. - Make sure to add the exact samehrefto theutils/i18n.tsin the re...(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 Markdownlint (0.37.0)
134-134: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
135-135: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
components/navigation/NavBar.tsx (2)
46-46: Consider improving language name handling.The language name transformation has a few areas for improvement:
- The hard-coded "English" string should use i18n for consistency
- The capitalization logic is duplicated across the component
- Missing validation against supported languages list
Consider extracting the language transformation logic into a utility function:
+ const transformLanguageName = (lang: string): string => { + const i18nKey = `languages.${lang.toLowerCase()}`; + const translatedName = t(i18nKey); + return translatedName.charAt(0).toUpperCase() + translatedName.slice(1); + }; const getUniqueLangs = (): string[] => { // ... existing path handling ... const uniqueLangs = Object.keys(i18nPaths) .filter((lang) => i18nPaths[lang].includes(pathnameWithoutLocale)) - .map((lang) => lang.charAt(0).toUpperCase() + lang.slice(1)); + .filter((lang) => languages.includes(lang.toLowerCase())) + .map(transformLanguageName); - return uniqueLangs.length === 0 ? ['English'] : uniqueLangs; + return uniqueLangs.length === 0 ? [transformLanguageName(defaultLanguage)] : uniqueLangs; };Also applies to: 59-62
236-236: Consolidate language name transformation logic.The capitalization logic is duplicated here. This should use the same utility function suggested for
getUniqueLangs.- selected={i18n.language ? i18n.language.charAt(0).toUpperCase() + i18n.language.slice(1) : 'English'} + selected={transformLanguageName(i18n.language || defaultLanguage)}
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ADDING_TRANSLATIONS.md(10 hunks)components/navigation/NavBar.tsx(6 hunks)
🧰 Additional context used
🪛 LanguageTool
ADDING_TRANSLATIONS.md
[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ng referenced. - Make sure to add the exact same href to the utils/i18n.ts in the re...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
[grammar] ~244-~244: Did you mean “exists”?
Context: ... people from all over the world. There exist a few locales like english (English) ...
(THERE_VBP_A_NN)
[style] ~250-~250: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... folder with the name of the locale you want to introduce. - Create new JSON files ...
(REP_WANT_TO_VB)
[style] ~251-~251: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...f the locale you want to introduce. - Create new JSON files with the same name as ...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Markdownlint (0.37.0)
ADDING_TRANSLATIONS.md
134-134: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
135-135: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
139-139: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
170-170: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
249-249: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
250-250: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
251-251: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
252-252: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
255-255: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
256-256: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
260-260: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
295-295: Expected: 0; Actual: 1
Unordered list indentation
(MD007, ul-indent)
248-248: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
254-254: null
Emphasis used instead of a heading
(MD036, no-emphasis-as-heading)
🔇 Additional comments (3)
ADDING_TRANSLATIONS.md (1)
Line range hint
297-312: LGTM: i18n paths configuration exampleThe i18n paths configuration example correctly demonstrates how to add routes for a new locale while maintaining consistency with existing patterns.
🧰 Tools
🪛 LanguageTool
[grammar] ~244-~244: Did you mean “exists”?
Context: ... people from all over the world. There exist a few locales likeenglish(English) ...(THERE_VBP_A_NN)
[style] ~250-~250: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... folder with the name of the locale you want to introduce. - Create newJSONfiles ...(REP_WANT_TO_VB)
[style] ~251-~251: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...f the locale you want to introduce. - Create newJSONfiles with the same name as ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 Markdownlint (0.37.0)
249-249: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
250-250: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
251-251: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
252-252: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
255-255: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
256-256: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
260-260: Expected: 0; Actual: 2
Unordered list indentation(MD007, ul-indent)
295-295: Expected: 0; Actual: 1
Unordered list indentation(MD007, ul-indent)
248-248: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
254-254: null
Emphasis used instead of a heading(MD036, no-emphasis-as-heading)
components/navigation/NavBar.tsx (2)
150-150: LGTM! Layout improvements enhance responsiveness.The spacing and alignment changes improve the UI in both mobile and desktop views.
Also applies to: 181-181
250-257: Verify mobile menu behavior during language switch.The mobile menu integration looks good, but there are two considerations:
- The currentLanguage transformation should use the suggested utility function
- Verify that the mobile menu state is properly reset after language switch
<MobileNavMenu onClickClose={() => setMobileMenuOpen(false)} uniqueLangs={uniqueLangs} - currentLanguage={i18n.language ? i18n.language.charAt(0).toUpperCase() + i18n.language.slice(1) : 'English'} + currentLanguage={transformLanguageName(i18n.language || defaultLanguage)} changeLanguage={changeLanguage} />✅ Verification successful
Mobile menu behavior during language switch is properly implemented.
The mobile menu automatically closes after language switch because:
- The menu state is reset via useEffect hook when the path changes
- Language changes trigger path updates through router navigation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check if mobile menu state is properly handled during language switch # Search for mobile menu state handling in MobileNavMenu rg -A 5 "MobileNavMenu.*onClickClose" --type tsx # Check for any useEffect hooks handling language changes rg "useEffect.*i18n\.language" --type tsxLength of output: 150
Script:
#!/bin/bash # Search for mobile menu implementation and language-related effects # Using broader file type matching # Check mobile menu implementation rg -t ts -t tsx -A 5 "MobileNavMenu.*onClickClose" # Look for language change handlers rg -t ts -t tsx "useEffect.*language" # Search for mobile menu state management rg -t ts -t tsx "setMobileMenuOpen" # Check MobileNavMenu component implementation fd -e ts -e tsx -x grep -l "MobileNavMenu" {} \; | xargs catLength of output: 17009
ADDING_TRANSLATIONS.md
Outdated
| - Navigate to the `next-i18next.config.js` file in the root of the project folder. | ||
| - Add the name of the newly added `locale` to the `languages` array. | ||
|
|
||
| **3. Configure i18n routing** | ||
| After adding a new internationalized page, ensure it is being served on the website when someone visits. | ||
| - Make sure to add the same `href` to the `lib/i18nPaths.js` in the respective locales supporting that `href`. | ||
| - Make sure to add the same `href` to the `utils/i18n.ts` in the respective locales supporting that `href`. | ||
|
|
||
| If you have added the 'fr' locale and translated the 'tools/cli' page, clicking 'Tools -> CLI' in the navigation menu will redirect the user to 'asyncapi.com/fr/tools/cli'. | ||
| If you have added the 'french' locale and translated the 'tools/cli' page, clicking 'Tools -> CLI' in the navigation menu will redirect the user to 'asyncapi.com/french/tools/cli'. | ||
|
|
||
| `locales` folder structure | ||
| ```diff | ||
| locales | ||
| ┣ de | ||
| ┣ deutsch | ||
| ┃ ┣ common.json | ||
| ┃ ┣ landing-page.json | ||
| ┃ ┗ tools.json | ||
| ┣ en | ||
| ┣ english | ||
| ┃ ┣ common.json | ||
| ┃ ┣ landing-page.json | ||
| ┃ ┗ tools.json | ||
| + ┗ fr | ||
| + ┗ french | ||
| + ┃ ┣ common.json | ||
| + ┃ ┣ landing-page.json | ||
| + ┃ ┗ tools.json | ||
| ``` | ||
|
|
||
| - Change the `next-i18next-static-site.config.js` config. | ||
| - Change the `next-i18next.config.js` config. | ||
|
|
||
| `next-i18next-static-site.config.js` | ||
| `next-i18next.config.js` | ||
| ```diff | ||
| module.exports = { | ||
| i18n: { | ||
| - languages: ["en", "de"], | ||
| + languages: ["en", "de", "fr"], | ||
| defaultLanguage: "en", | ||
| - languages: ["english", "deutsch"], | ||
| + languages: ["english", "deutsch", "french"], | ||
| defaultLanguage: "english", |
There was a problem hiding this comment.
💡 Codebase verification
Documentation inconsistency confirmed
The documentation refers to two different configuration files:
next-i18next-static-site.config.js(incorrect reference)next-i18next.config.js(actual file)
The correct configuration file name is next-i18next.config.js, as verified in the repository. The reference to next-i18next-static-site.config.js should be updated to maintain consistency.
🔗 Analysis chain
Verify configuration file references
The documentation refers to next-i18next.config.js, but earlier sections mention next-i18next-static-site.config.js. This inconsistency needs to be addressed.
Let's verify the correct configuration file name:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check which configuration file exists in the repository
fd -e js -E 'node_modules' 'next-i18next.*config'
Length of output: 72
Script:
#!/bin/bash
# Search for any mentions of the static site config file
rg "next-i18next-static-site\.config"
Length of output: 159
🧰 Tools
🪛 Markdownlint (0.37.0)
255-255: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
256-256: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
260-260: Expected: 0; Actual: 2
Unordered list indentation
(MD007, ul-indent)
|
@asyncapi/bounty_team |
public/locales/deutsch/common.json
Outdated
There was a problem hiding this comment.
IMHO, we should keep it as de and en. I haven't encountered a website using full names in URLs.
The dropdown should have full names like Deutsch but the URLs should still point to /de or /en and not the full /deutsch
There was a problem hiding this comment.
@sambhavgupta0705 suggested this change in the #3113. But I can revert this change.
There was a problem hiding this comment.
Please revert the changes related to en -> english here and in i18n config file since URLs should be having smaller prefix
There was a problem hiding this comment.
Okay I will do that!
|
The design-related changes looks good |
Co-authored-by: Ansh Goyal <[email protected]>
|
@anshgoyalevil Do I need to fix the Lighthouse Failing errors in this PR due to the image and CSS part: |
|
Yes, If the regression is caused by this PR (but I don't think it is). Otherwise, you can ignore it and solve (if willing) it in another PR. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
components/icons/Language.tsx (2)
4-6: Enhance JSDoc documentation with props description.Add
@paramand@returnstags to document the component's interface./** * @description Language Icon for language selector component + * @param {Object} props - Component props + * @param {string} [props.className=''] - Additional CSS classes to apply + * @returns {JSX.Element} Language icon SVG */
7-7: Add TypeScript type definitions.Since this is a TypeScript file, add proper type definitions for the component props.
-export default function IconLanguage({ className = '' }) { +interface IconLanguageProps { + className?: string; +} + +export default function IconLanguage({ className = '' }: IconLanguageProps) {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/icons/Language.tsx(1 hunks)
🔇 Additional comments (2)
components/icons/Language.tsx (2)
3-3: Consider removing eslint-disable comment.The
max-lendisable comment might be unnecessary if the SVG path can be formatted across multiple lines.Let's check if other icon components use similar eslint-disable comments:
9-23: SVG implementation looks good!The SVG implementation follows best practices:
- Uses currentColor for dynamic color inheritance
- Allows className customization
- Maintains consistent stroke width
- Uses appropriate viewBox dimensions
Note: The
size-5class suggests Tailwind CSS usage. Ensure this utility class is available in your Tailwind configuration.Let's verify the Tailwind configuration:
✅ Verification successful
Tailwind size utility verification successful
The
size-5utility class is valid and follows the project's established patterns. Tailwind CSS is properly configured in the project, and similar size utilities are consistently used across 40+ components.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check Tailwind configuration for size utilities rg -l "size-" "tailwind.config.*"Length of output: 91
Script:
#!/bin/bash # Check for Tailwind dependencies and configuration echo "=== Package.json check ===" fd -t f "package.json" --exec cat {} \; | grep -i "tailwind" echo -e "\n=== Searching for Tailwind config files ===" fd -t f "(tailwind|postcss)\.config\." --hidden echo -e "\n=== Checking size utility usage ===" rg "size-[0-9]" -lLength of output: 1996
|
@anshgoyalevil I have reverted the changes related to |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
components/languageSelector/LanguageSelect.tsx (1)
16-34: Enhance accessibility of the language selector.While the visual layout with the icon looks good, let's improve accessibility:
- Add aria-label to the select element
- Add aria-hidden to the icon since it's decorative
<IconLanguage className='pointer-events-none absolute left-3 text-gray-600' /> <select data-testid='Select-form' + aria-label='Select language' onChange={(ev) => onChange(ev.target.value)} className={twMerge( `form-select h-full px-10 pr-7 inline-flex justify-center rounded-md border border-gray-300 shadow-sm py-2 bg-white text-sm font-medium text-gray-700 hover:bg-gray-50 focus:border-gray-500 focus:outline-none focus:ring-0 focus:ring-black ${className}` )} value={selected} >next-i18next.config.js (1)
9-9: Remove unnecessary empty line.The empty line after the i18n configuration object is unnecessary and should be removed to maintain consistent formatting.
i18n: { locales: ['en', 'de'], defaultLocale : 'en', namespaces: ['landing-page', 'common', 'tools'], defaultNamespace: 'landing-page', react: { useSuspense: false },// this line }, - };🧰 Tools
🪛 ESLint
[error] 9-10: Delete
⏎··(prettier/prettier)
ADDING_TRANSLATIONS.md (1)
134-135: Improve wording in the documentation.The phrase "exact same" is wordy. Consider simplifying the instruction.
- - Make sure to add the exact same `href` to the `utils/i18n.ts` in the respective locales which support that `href`. + - Add the matching `href` to the `utils/i18n.ts` in the respective locales which support that `href`.🧰 Tools
🪛 LanguageTool
[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ng referenced. - Make sure to add the exact samehrefto theutils/i18n.tsin the re...(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 markdownlint-cli2 (0.17.2)
134-134: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
ADDING_TRANSLATIONS.md(8 hunks)components/languageSelector/LanguageSelect.tsx(2 hunks)components/navigation/MobileNavMenu.tsx(4 hunks)components/navigation/NavBar.tsx(4 hunks)next-i18next.config.js(1 hunks)
🧰 Additional context used
🪛 LanguageTool
ADDING_TRANSLATIONS.md
[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ng referenced. - Make sure to add the exact same href to the utils/i18n.ts in the re...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 markdownlint-cli2 (0.17.2)
ADDING_TRANSLATIONS.md
134-134: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
170-170: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
255-255: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
256-256: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
260-260: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
295-295: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
🪛 ESLint
components/navigation/MobileNavMenu.tsx
[error] 33-33: Delete ·
(prettier/prettier)
components/languageSelector/LanguageSelect.tsx
[error] 14-14: Delete ·
(prettier/prettier)
next-i18next.config.js
[error] 9-10: Delete ⏎··
(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
components/navigation/MobileNavMenu.tsx
[error] 33-33: Delete whitespace (prettier/prettier)
components/languageSelector/LanguageSelect.tsx
[error] 14-14: Delete whitespace (prettier/prettier)
🔇 Additional comments (4)
components/navigation/MobileNavMenu.tsx (1)
139-161: Improve accessibility of the language selection menu.The current implementation has accessibility issues that need to be addressed.
components/navigation/NavBar.tsx (1)
46-46: LGTM!The changes look good:
- JSDoc accurately reflects the function behavior
- Layout spacing improvements enhance visual hierarchy
- Language-related props are correctly passed to MobileNavMenu
Also applies to: 150-150, 181-181, 250-257
next-i18next.config.js (1)
Line range hint
2-7: Verify locale codes are using short forms.The configuration correctly uses short locale codes ('en', 'de') which aligns with the URL requirements. This is the correct approach as confirmed in the past review comments.
Let's verify the locale codes usage across the codebase:
✅ Verification successful
Short locale codes are consistently used throughout the codebase
The verification confirms that only short locale codes ('en', 'de') are used consistently across all i18n configurations and implementations. No instances of full language names or non-standard locale codes were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any inconsistent locale code usage # Search for full locale names that should be using short codes rg -i "defaultLocale.*(?:english|deutsch)" --type ts --type js rg -i "locales.*(?:english|deutsch)" --type ts --type jsLength of output: 123
Script:
#!/bin/bash # Search for locale-related configurations and potential inconsistencies echo "=== Searching for locale configurations ===" rg -i "locale.*=.*['\"].*['\"]" --type ts --type js echo -e "\n=== Checking for language codes longer than 2 characters ===" rg -i "locale.*=.*['\"][a-zA-Z]{3,}['\"]" --type ts --type js echo -e "\n=== Looking for i18n usage patterns ===" rg "useTranslation|i18n\..*locale" --type ts --type jsLength of output: 3134
🧰 Tools
🪛 ESLint
[error] 6-6: Delete
··(prettier/prettier)
[error] 7-7: Replace
··react:·{·useSuspense:·false·},withreact:·{·useSuspense:·false·}·(prettier/prettier)
[error] 8-8: Replace
··},with}(prettier/prettier)
[error] 9-10: Delete
⏎··(prettier/prettier)
ADDING_TRANSLATIONS.md (1)
255-256: Update configuration file reference.The documentation refers to
next-i18next.config.jswhich is correct, but this needs to be consistently used throughout the document.🧰 Tools
🪛 markdownlint-cli2 (0.17.2)
255-255: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
256-256: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
| @@ -10,21 +11,27 @@ import type { SelectProps } from '../form/Select'; | |||
| * @param {Array} [props.options=[]] - An array of options for the select dropdown. | |||
| * @param {string} props.selected - The currently selected option value. | |||
| */ | |||
| export default function LanguageSelect({ className = '', onChange = () => {}, options = [], selected }: SelectProps) { | |||
| export default function LanguageSelect({ className = '', onChange = () => { }, options = [], selected }: SelectProps) { | |||
There was a problem hiding this comment.
Fix formatting issue.
Remove the extra space after the opening curly brace to resolve the prettier error.
-export default function LanguageSelect({ className = '', onChange = () => { }, options = [], selected }: SelectProps) {
+export default function LanguageSelect({ className = '', onChange = () => {}, options = [], selected }: SelectProps) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export default function LanguageSelect({ className = '', onChange = () => { }, options = [], selected }: SelectProps) { | |
| export default function LanguageSelect({ className = '', onChange = () => {}, options = [], selected }: SelectProps) { |
🧰 Tools
🪛 ESLint
[error] 14-14: Delete ·
(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
[error] 14-14: Delete whitespace (prettier/prettier)
| `next-i18next.config.js` | ||
| ```diff | ||
| module.exports = { | ||
| i18n: { | ||
| - languages: ["en", "de"], | ||
| + languages: ["en", "de", "fr"], | ||
| defaultLanguage: "en", | ||
| defaultLanguage: "english", | ||
| namespaces: ["landing-page", "common", "tools"], | ||
| defaultNamespace: "landing-page", | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Revert locale names to short forms in examples.
Based on the past review comments, we should use short locale codes instead of full names in the examples.
module.exports = {
i18n: {
- languages: ["en", "de", "fr"],
+ locales: ["en", "de", "fr"],
- defaultLanguage: "english",
+ defaultLocale: "en",
namespaces: ["landing-page", "common", "tools"],
defaultNamespace: "landing-page",
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| `next-i18next.config.js` | |
| ```diff | |
| module.exports = { | |
| i18n: { | |
| - languages: ["en", "de"], | |
| + languages: ["en", "de", "fr"], | |
| defaultLanguage: "en", | |
| defaultLanguage: "english", | |
| namespaces: ["landing-page", "common", "tools"], | |
| defaultNamespace: "landing-page", | |
| }, | |
| }; | |
| module.exports = { | |
| i18n: { | |
| locales: ["en", "de", "fr"], | |
| defaultLocale: "en", | |
| namespaces: ["landing-page", "common", "tools"], | |
| defaultNamespace: "landing-page", | |
| }, | |
| }; |
|
|
||
| If you have added the 'fr' locale and translated the 'tools/cli' page, clicking 'Tools -> CLI' in the navigation menu will redirect the user to 'asyncapi.com/fr/tools/cli'. | ||
| If you have added the 'french' locale and translated the 'tools/cli' page, clicking 'Tools -> CLI' in the navigation menu will redirect the user to 'asyncapi.com/french/tools/cli'. |
There was a problem hiding this comment.
| If you have added the 'french' locale and translated the 'tools/cli' page, clicking 'Tools -> CLI' in the navigation menu will redirect the user to 'asyncapi.com/french/tools/cli'. | |
| If you have added the 'fr' locale and translated the 'tools/cli' page, clicking 'Tools -> CLI' in the navigation menu will redirect the user to 'asyncapi.com/fr/tools/cli'. |
| ```diff | ||
| module.exports = { | ||
| i18n: { | ||
| - languages: ["en", "de"], | ||
| + languages: ["en", "de", "fr"], | ||
| defaultLanguage: "en", | ||
| defaultLanguage: "english", |
There was a problem hiding this comment.
| defaultLanguage: "english", | |
| defaultLanguage: "en", |
|
My point was to keep full-language names here while keeping the initials (like
|
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ADDING_TRANSLATIONS.md (1)
283-294:⚠️ Potential issuei18n Config: French Locale Addition
The configuration now includesfrin thelanguagesarray, which is appropriate for expanding supported locales. However, thedefaultLanguageremains set to"english". Given previous guidance to use short locale codes for URLs, please updatedefaultLanguageto"en".
🧹 Nitpick comments (2)
ADDING_TRANSLATIONS.md (2)
134-137: Routing Instructions Revision
The instructions now advise:
• Replacing thenext/linkcomponent withLinkComponentfromcomponents/link.tsx
• Ensuring that an identicalhrefis added toutils/i18n.tsfor the supported localesSuggestion: Consider replacing the phrase "exact same" with "identical" for conciseness. Also, verify that the bullet list indentation conforms with markdown guidelines.
🧰 Tools
🪛 LanguageTool
[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ng referenced. - Make sure to add the exact samehrefto theutils/i18n.tsin the re...(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 markdownlint-cli2 (0.17.2)
134-134: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
1-315: Markdown Formatting Note
Multiple unordered list indentation warnings (MD007) have been flagged by static analysis (e.g., at lines 134, 135, 170, 255-256, 260, and 295). Consider reviewing and adjusting these indentations to ensure consistency with markdown style guidelines.🧰 Tools
🪛 LanguageTool
[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ng referenced. - Make sure to add the exact samehrefto theutils/i18n.tsin the re...(EN_WORDINESS_PREMIUM_EXACT_SAME)
[grammar] ~244-~244: Did you mean “exists”?
Context: ... people from all over the world. There exist a few locales likeen(English) and `...(THERE_VBP_A_NN)
[style] ~250-~250: You have already used this phrasing in nearby sentences. Consider replacing it to add variety to your writing.
Context: ... folder with the name of the locale you want to introduce. - Create newJSONfiles ...(REP_WANT_TO_VB)
[style] ~251-~251: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...f the locale you want to introduce. - Create newJSONfiles with the same name as ...(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.17.2)
19-19: Fenced code blocks should have a language specified
null(MD040, fenced-code-language)
99-99: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
100-100: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
101-101: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
102-102: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
104-104: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
105-105: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
106-106: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
108-108: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
109-109: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
110-110: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
130-130: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
130-130: Link fragments should be valid
null(MD051, link-fragments)
134-134: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
139-139: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
170-170: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
218-218: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
248-248: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
249-249: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
250-250: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
251-251: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
252-252: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
254-254: Emphasis used instead of a heading
null(MD036, no-emphasis-as-heading)
255-255: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
256-256: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
260-260: Unordered list indentation
Expected: 0; Actual: 2(MD007, ul-indent)
295-295: Unordered list indentation
Expected: 0; Actual: 1(MD007, ul-indent)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ADDING_TRANSLATIONS.md(9 hunks)components/languageSelector/LanguageSelect.tsx(2 hunks)components/navigation/MobileNavMenu.tsx(4 hunks)pages/community/index.tsx(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- pages/community/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- components/languageSelector/LanguageSelect.tsx
- components/navigation/MobileNavMenu.tsx
🧰 Additional context used
🪛 LanguageTool
ADDING_TRANSLATIONS.md
[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative.
Context: ...ng referenced. - Make sure to add the exact same href to the utils/i18n.ts in the re...
(EN_WORDINESS_PREMIUM_EXACT_SAME)
🪛 markdownlint-cli2 (0.17.2)
ADDING_TRANSLATIONS.md
134-134: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
135-135: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
170-170: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
255-255: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
256-256: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
260-260: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
295-295: Unordered list indentation
Expected: 0; Actual: 1
(MD007, ul-indent)
⏰ Context from checks skipped due to timeout of 180000ms (3)
- GitHub Check: Redirect rules - asyncapi-website
- GitHub Check: Header rules - asyncapi-website
- GitHub Check: Pages changed - asyncapi-website
🔇 Additional comments (9)
ADDING_TRANSLATIONS.md (9)
6-9: TOC Update: New Sections Added
The updated Table of Contents now includes the added sections for "Improving existing translations," "Adding translations to a partially localized page," "Adding translations to a new page," and "Adding a new locale." This update improves navigation within the document.
48-48: Updated Import Instruction
The documentation now instructs to import theuseTranslationhook fromutils/i18n, which is consistent with the refactored file structure.
56-75: ICSFileButton Example Update
The diff example forICSFileButton.jscorrectly:
• Updates the import to use../../utils/i18n
• Replaces the literal button text with a translation key (icsFileBtn)
• Introduces the use of the translation function (t()) fromuseTranslation
This encourages proper internationalization practices in component development.
79-82: English Locale Update
Adding the"icsFileBtn": "Download ICS File"key inen/common.jsonensures that the English translation for the button is available for i18n.
86-89: German Locale Update
The addition of"icsFileBtn": "ICS-Datei herunterladen"inde/common.jsoncompletes the localized mapping for the button in German.
143-154: Locales Directory Update
The diff shows new additions ofnewsletter.jsonin both thedeandenlocale folders. This change aligns the directory structure with the new newsletter page translations.
159-168: i18n Config Update: Newsletter Namespace
Including thenewsletternamespace in thenamespacesarray of the i18n configuration is implemented correctly. Ensure that any components relying on this namespace are updated accordingly.
182-216: Newsletter Page i18n Integration
The updatednewsletter.tsxexample properly:
• Imports i18n utilities (getAllLanguageSlugs,getLanguage, anduseTranslation)
• Uses theuseTranslationhook with the'newsletter'namespace
• Implements static site functions (getStaticPathsandgetStaticProps)
This mirrors the approach used elsewhere in the project and supports full i18n integration for the newsletter page.
297-312: i18n Routing Update for French
The diff forutils/i18n.tscorrectly adds a French path (/tools/cli) under thefrkey, aligning with the updated i18n configuration.
Description
This PR enables the i18n feature for smaller screens. Additionally, the following changes have been made:
Related issue(s)
Resolves #2708
Summary by CodeRabbit
Release Notes
New Features
Improvements
IconLanguage.Documentation