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
WalkthroughA new language icon component, IconLanguage, has been added to the codebase and integrated across multiple areas. The icon is introduced into the icons gallery, and the language selection components have been updated for enhanced mobile support. Both the mobile navigation (via new props in MobileNavMenu and updates in NavBar) and the LanguageSelect component now incorporate the new icon, adjusted layout, and language mapping. Additionally, the i18n configuration and translation documentation have been updated to reflect new naming conventions and file paths. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MobileNavMenu
participant ChangeLanguageHandler
User->>MobileNavMenu: Tap on a language option
MobileNavMenu->>MobileNavMenu: Determine selected language
MobileNavMenu->>ChangeLanguageHandler: Invoke changeLanguage(selectedLang)
ChangeLanguageHandler-->>MobileNavMenu: Update language state
MobileNavMenu-->>User: Display updated language setting
Assessment against linked issues
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (2)
🧰 Additional context used🪛 LanguageToolADDING_TRANSLATIONS.md[style] ~135-~135: ‘exact same’ might be wordy. Consider a shorter alternative. (EN_WORDINESS_PREMIUM_EXACT_SAME) 🪛 markdownlint-cli2 (0.17.2)ADDING_TRANSLATIONS.md134-134: Unordered list indentation (MD007, ul-indent) 135-135: Unordered list indentation (MD007, ul-indent) 170-170: Unordered list indentation (MD007, ul-indent) 255-255: Unordered list indentation (MD007, ul-indent) 256-256: Unordered list indentation (MD007, ul-indent) 260-260: Unordered list indentation (MD007, ul-indent) 300-300: Unordered list indentation (MD007, ul-indent) 🪛 ESLintnext-i18next.config.js[error] 7-7: Replace (prettier/prettier) [error] 11-11: Delete (prettier/prettier) [error] 12-12: Delete (prettier/prettier) ⏰ Context from checks skipped due to timeout of 180000ms (1)
🔇 Additional comments (3)
✨ Finishing Touches
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 <anshgoyal1704@gmail.com>
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)
|
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.
|
@devilkiller-ag kindly resolve the comments |
Done all requested changes! |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/languageSelector/LanguageSelect.tsx (1)
14-14: 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) {🧰 Tools
🪛 ESLint
[error] 14-14: Delete
·(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
[error] 14-14: Delete
·prettier/prettiercomponents/navigation/MobileNavMenu.tsx (1)
144-166: Improve accessibility of the language selection menu.The current implementation has accessibility issues:
- Using a div with onClick instead of a button
- Missing ARIA attributes for the expandable menu
- <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' > {langMap[lang.text.toLowerCase()] || lang.text} </button> - ))} + )} + </div> </div> </div> + </button> </div>📝 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.<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> <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' > {langMap[lang.text.toLowerCase()] || lang.text} </button> ))} </div> </div> </div> </button> </div>
🧹 Nitpick comments (5)
ADDING_TRANSLATIONS.md (2)
6-9: Table of Contents Formatting CheckThe updated table of contents entries are clear and descriptive. However, a few markdownlint warnings suggest that the unordered list indentation might be off. Please verify that the list items start at the beginning of the line with no unintended leading spaces.
134-137: i18n Routing Instruction ClarityThe instructions to replace the
next/linkcomponent with the customLinkComponentand to "add the exact samehref" are clear overall. As a minor stylistic improvement, consider replacing “exact same” with “identical” to reduce wordiness, per the style hint.🧰 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)
components/languageSelector/LanguageSelect.tsx (2)
15-18: Consider moving langMap to a shared configuration.The
langMapobject is duplicated in multiple components. Consider moving it to a shared configuration file, possibly alongside other i18n configurations.Create a new file
utils/i18n/langMap.ts:export const langMap: { [key: string]: string } = { en: 'English', de: 'Deutsch' };
25-38: Enhance select element accessibility.The language selector could benefit from improved accessibility attributes.
<select data-testid='Select-form' onChange={(ev) => onChange(ev.target.value)} + aria-label="Select language" + role="combobox" 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} >components/navigation/NavBar.tsx (1)
48-62: Consider enhancing getUniqueLangs function.The function could be more robust by:
- Validating input against supported languages
- Memoizing results for performance
+import { useMemo } from 'react'; + const getUniqueLangs = (): string[] => { + return useMemo(() => { let pathnameWithoutLocale = pathname; // Check if the pathname includes "/[lang]", if so, replace it with an empty string if (pathname && pathname.includes('/[lang]')) { pathnameWithoutLocale = pathname.replace('/[lang]', ''); } // Filter unique languages based on i18nPaths that include the modified pathnameWithoutLocale const uniqueLangs = Object.keys(i18nPaths) - .filter((lang) => i18nPaths[lang].includes(pathnameWithoutLocale)); + .filter((lang) => + languages.includes(lang) && i18nPaths[lang].includes(pathnameWithoutLocale) + ); // If no unique languages are found, default to ['en'] return uniqueLangs.length === 0 ? ['en'] : uniqueLangs; + }, [pathname]); };🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 ESLint
[error] 57-58: Delete
⏎······(prettier/prettier)
🪛 GitHub Actions: PR testing - if Node project
[error] 57-57: Delete
⏎······prettier/prettier
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
ADDING_TRANSLATIONS.md(10 hunks)components/languageSelector/LanguageSelect.tsx(2 hunks)components/navigation/MobileNavMenu.tsx(5 hunks)components/navigation/NavBar.tsx(6 hunks)
🧰 Additional context used
🪛 GitHub Actions: PR testing - if Node project
components/navigation/NavBar.tsx
[error] 57-57: Delete ⏎······ prettier/prettier
components/navigation/MobileNavMenu.tsx
[error] 33-33: Delete · prettier/prettier
components/languageSelector/LanguageSelect.tsx
[error] 14-14: Delete · prettier/prettier
🪛 ESLint
components/navigation/MobileNavMenu.tsx
[error] 33-33: Delete ·
(prettier/prettier)
components/languageSelector/LanguageSelect.tsx
[error] 14-14: Delete ·
(prettier/prettier)
🪛 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)
🔇 Additional comments (6)
ADDING_TRANSLATIONS.md (5)
56-75: ICSFileButton.js Example – i18n Hook UpdateThe diff correctly updates the import to use the new path (
'../../utils/i18n'), changes the default text from a hard-coded string to a translation key ('icsFileBtn'), and applies the translation function viat(). This improves consistency in localization. Ensure that the translation key in your JSON files exactly matches (icsFileBtn).
79-89: Translation Keys in JSON FilesThe additions in both
en/common.jsonandde/common.jsonappropriately add the"icsFileBtn"key with their respective translations. This change aligns with the adjustment in the component.
182-216: Newsletter Page Localization ImplementationThe changes in the newsletter page example are well structured. The diff shows:
- An updated import from
"../../utils/i18n"for the necessary i18n functions.- The proper use of the
useTranslation('newsletter')hook.- Implementation of both
getStaticPathsandgetStaticPropsfollowing a common pattern.Please ensure that the new namespace (
newsletter) is also added to the i18n configuration (see the configuration diff below) so that translations for this page are correctly resolved.
283-294: i18n Configuration UpdateThe diff in
next-i18next.config.jsnow adds the French locale ("fr") to thelanguagesarray and correctly updates thenamespacesarray to include"newsletter". This aligns with the new translation requirements for the newsletter page. Double-check that any related routing or default language settings remain consistent.
297-312: utils/i18n.ts – Locale Paths ConsistencyThe diff updates
utils/i18n.tsby adding a new entry for the French locale:
- For English and German, earlier diff sections added the
/newsletterpath along with/tools/cli, but in this block the arrays forenanddeonly list/tools/cliwhile French now only has/tools/cli.There is a potential inconsistency here. If
/newsletteris intended to be available for all locales, please ensure that the French locale (as well as the English and German entries in this updated diff) also include the/newsletterroute. If this is intentional (to be configured separately), kindly verify and document the difference.components/navigation/MobileNavMenu.tsx (1)
53-56: Move langMap to a shared configuration.The
langMapobject is duplicated across components.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/languageSelector/LanguageSelect.tsx(2 hunks)components/navigation/MobileNavMenu.tsx(5 hunks)components/navigation/NavBar.tsx(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/languageSelector/LanguageSelect.tsx
- components/navigation/MobileNavMenu.tsx
🔇 Additional comments (5)
components/navigation/NavBar.tsx (5)
57-57: LGTM!The code formatting has been fixed as requested in the previous review.
59-60: LGTM!The default language handling now consistently uses lowercase ('en'), which aligns with the i18n standards and the PR objectives.
248-255: LGTM!The mobile navigation menu has been properly enhanced with language selection functionality, implementing the i18n feature for smaller screens as intended.
148-148: LGTM!The layout adjustments improve the spacing and responsiveness while maintaining consistent use of Tailwind classes.
Also applies to: 179-179
234-234: LGTM!The language selection implementation consistently handles the default language across both desktop and mobile views.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
components/languageSelector/LanguageSelect.tsx (4)
20-24: Enhance accessibility for screen readers.While the language icon is properly marked as non-interactive, consider adding ARIA labels to improve screen reader experience.
Apply this diff to enhance accessibility:
- <div className='relative inline-block'> + <div className='relative inline-block' role="presentation"> <div className='relative flex items-center gap-2'> {/* Display Icon Next to the Select Box */} - <IconLanguage className='pointer-events-none absolute left-3 text-gray-600' /> + <IconLanguage className='pointer-events-none absolute left-3 text-gray-600' aria-hidden="true" />
24-31: Add aria-label for language selection.The select element should have an aria-label to clearly indicate its purpose to screen readers.
Apply this diff to add the aria-label:
<select data-testid='Select-form' onChange={(ev) => onChange(ev.target.value)} + aria-label="Select language" 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}
32-36: Improve mobile responsiveness.The current implementation looks good, but consider adding touch-friendly spacing for mobile devices.
Apply this diff to enhance mobile responsiveness:
{options.map((option, index) => ( <option key={index} value={option.value} data-testid='Option-form'> {langMap[option.text.toLowerCase() as keyof typeof langMap] || option.text} </option> ))} + {/* Add mobile-friendly padding */} + <style jsx>{` + @media (max-width: 640px) { + select { + padding-top: 0.75rem; + padding-bottom: 0.75rem; + } + } + `}</style>
17-17: Consider memoizing langMap extraction.Since langMap is static, consider memoizing its extraction to prevent unnecessary re-renders.
Apply this diff to optimize performance:
+ const langMap = React.useMemo(() => i18nextConfig.langMap, []); - const { langMap } = i18nextConfig;
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/languageSelector/LanguageSelect.tsx(2 hunks)components/navigation/MobileNavMenu.tsx(5 hunks)next-i18next.config.js(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- components/navigation/MobileNavMenu.tsx
🧰 Additional context used
🪛 ESLint
next-i18next.config.js
[error] 7-7: Replace , with ·
(prettier/prettier)
[error] 12-12: Delete ,
(prettier/prettier)
⏰ 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 (1)
next-i18next.config.js (1)
3-3: Verify language codes against ISO standards.Ensure that the language codes and their mappings comply with ISO 639-1 standards.
Also applies to: 9-11
✅ Verification successful
🌐 Web query:
What are the ISO 639-1 language codes for English and German?💡 Result:
The ISO 639-1 language codes for English and German are standardized two-letter identifiers used globally:
- English:
en- German:
deThese codes are part of the ISO 639-1 standard, which provides concise identifiers for major languages[1][4][7][9]. For example:
en-USrepresents English as spoken in the United States[1]de-CHrepresents German as used in Switzerland[3]The standard allows combining these language codes with country codes (ISO 3166) or script codes (ISO 15924) to specify regional or script variations[1][3].
Citations:
- 1: https://localizely.com/iso-639-1-list/
- 2: http://xml.coverpages.org/iso639a.html
- 3: https://www.phpkb.com/kb/article/iso-639-1-standard-language-codes-255.html
- 4: https://en.wikipedia.org/wiki/List_of_ISO_639_language_codes
- 5: https://datahub.io/core/language-codes
- 6: https://gist.github.com/Josantonius/b455e315bc7f790d14b136d61d9ae469
- 7: https://lingohub.com/blog/what-are-iso-639-language-codes
- 8: https://quickref.me/iso-639-1.html
- 9: https://en.wikipedia.org/wiki/ISO_639-1
- 10: https://www.iso.org/iso-639-language-code
Language codes are correctly set.
The ISO 639-1 language codes for English and German are correctly specified as
enanddein the configuration file (lines 3 and applicable lines 9-11).
|
/rtm |
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
New Features
Documentation