Skip to content

Conversation

wackerow
Copy link
Member

@wackerow wackerow commented Aug 1, 2025

Description

  • Restores functionality that surfaced full list of browser language preferences to the top of the language picker.

Preview link

https://deploy-preview-16000--ethereumorg.netlify.app/es

cc: @konopkja

Copy link

netlify bot commented Aug 1, 2025

Deploy Preview for ethereumorg ready!

Name Link
🔨 Latest commit 2e0e912
🔍 Latest deploy log https://app.netlify.com/projects/ethereumorg/deploys/688ced99df7612000869d75d
😎 Deploy Preview https://deploy-preview-16000--ethereumorg.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.
Lighthouse
Lighthouse
7 paths audited
Performance: 56 (🟢 up 5 from production)
Accessibility: 90 (🔴 down 4 from production)
Best Practices: 92 (no change from production)
SEO: 97 (🔴 down 2 from production)
PWA: -
View the detailed breakdown and full score reports

To edit notification comments on pull requests, go to your Netlify project configuration.

@konopkja
Copy link
Contributor

konopkja commented Aug 8, 2025

lgtm

@minimalsm
Copy link
Contributor

@claude review plz

Copy link
Contributor

claude bot commented Aug 8, 2025

Claude finished @minimalsm's task —— View job


Code Review

Reviewing the language picker fix to restore browser language preferences functionality.

Todo List:

  • Read and analyze the changed file
  • Review code quality and implementation
  • Check for potential issues or improvements
  • Provide comprehensive feedback

Review Feedback

✅ Overall Assessment: Good Fix

This PR successfully restores the functionality to surface all browser language preferences at the top of the language picker. The implementation is well-structured and handles edge cases appropriately.

Strengths:

  1. Comprehensive Language Matching (src/components/LanguagePicker/useLanguagePicker.tsx:28-43)

    • Proper handling of both exact matches and partial matches (e.g., 'en-US' → 'en')
    • Case-insensitive matching for robustness
    • Prevents duplicate preferences with !preferences.includes(match) check
  2. Backward Compatibility (src/components/LanguagePicker/useLanguagePicker.tsx:46)

    • Maintains intlLocalePreference for existing code that expects single preference
    • Clean migration from single to multiple preferences
  3. Smart Sorting Logic (src/components/LanguagePicker/useLanguagePicker.tsx:65-84)

    • All browser preferences float to the top
    • Browser preferences maintain their original preference order
    • Non-preference languages sorted alphabetically with proper locale-aware comparison
  4. Performance Optimization

    • Uses useMemo appropriately to prevent unnecessary recalculations
    • Dependencies are correctly specified in memo arrays

Code Quality Notes:

  1. Type Safety - Good use of TypeScript with proper type assertions and null checks
  2. SSR Compatibility - Safely handles navigator object with typeof navigator !== "undefined" check
  3. Clear Variable Naming - intlLocalePreferences vs intlLocalePreference clearly distinguishes plural/singular

Minor Suggestions:

  1. Comment Clarity (src/components/LanguagePicker/useLanguagePicker.tsx:24)

    • Consider fixing typo: "users browser" → "user's browser"
  2. Performance Consideration (src/components/LanguagePicker/useLanguagePicker.tsx:66-70)

    • The indexOf calls in sort could be optimized by creating a preference index map, but given typical browser language list sizes (≤10), current approach is fine

Testing Considerations:

  • The preview link shows the fix working correctly with Spanish locale
  • Multiple browser language preferences should now appear at the top in correct order
  • Fallback behavior for users without matching preferences works as expected

Recommendation: ✅ Approve

This is a solid fix that restores important functionality while maintaining backward compatibility and code quality standards.


@corwintines corwintines merged commit 4d1fc6d into dev Aug 26, 2025
7 checks passed
@corwintines corwintines deleted the navigator-languages branch August 26, 2025 23:22
This was referenced Aug 27, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants