Skip to content

Conversation

@agent-kal
Copy link

@agent-kal agent-kal bot commented Jul 15, 2025

We want the frontend of the greetings project to be way more fun. To do this, when a greeting is shown in any language, we want to show a globe indicating where that language is primarily spoken.

Localization

The backend needs to provide the proper locale for each language. The greetings.json should include a locale field, and the APIs that return greetings should include that locale in the API responses

Display

Using a cool 3d globe, show me where the locale of the language corresponds to in the world. Make sure to use a simple solution so that its easy to maintain and ensure its accuracy.

Completed by Agent
Fixes #155

@github-actions
Copy link
Contributor

Test this PR by running the following command:

dagger -m github.com/kpenfound/greetings-api@pull/156/head call check

Run this branch locally:

dagger -m github.com/kpenfound/greetings-api@pull/156/head call serve up

@agent-kal
Copy link
Author

agent-kal bot commented Jul 15, 2025

Pull Request Review

General Opinion

This pull request successfully implements the requested globe visualization feature for the greetings project. The implementation adds locale information to the backend API and creates an impressive 3D globe visualization using Three.js that displays country locations for different languages. The changes are well-structured and accomplish the stated objectives.

Positive Aspects

  1. Complete Implementation: The PR fully addresses both backend and frontend requirements:

    • Added locale fields to greetings.json with appropriate country codes
    • Updated Go structs and API responses to include locale information
    • Created a sophisticated 3D globe visualization using Three.js
  2. Test Coverage: The existing tests were properly updated to reflect the new locale field, and all tests are passing.

  3. Visual Appeal: The globe implementation includes nice touches like:

    • Procedural earth texture with continents
    • Atmospheric glow effect
    • Animated markers with pulsing effect
    • Smooth globe rotation
  4. Error Handling: The frontend gracefully handles missing locale data by showing a fallback message.

Required Changes

  1. API Endpoint Inconsistency: The frontend is fetching from http://localhost:8080/ but the diff shows it was originally fetching from http://localhost:8080/random. This change wasn't mentioned in the PR description and may break existing functionality. The /random endpoint should be restored or this change should be documented.

  2. Missing Newline: The greetings.json file is missing a trailing newline, which can cause issues with some tools and goes against common conventions.

  3. Incomplete Test Coverage: The end-to-end tests don't verify the new globe functionality. At minimum, tests should verify:

    • The globe container appears after clicking the button
    • The location info displays correctly
    • The API response includes the locale field

Optional Improvements

  1. Memory Management: The globe animation creates multiple animation frames that could potentially cause memory leaks. Consider cleaning up the animation when the component is no longer needed.

  2. Accessibility: The globe visualization lacks accessibility features. Consider adding alt text, keyboard navigation, or screen reader support.

  3. Performance: Loading Three.js from CDN on every page load could be optimized, though this is acceptable for a demo project.

  4. Coordinate Data: Some locale choices could be debated (e.g., Arabic mapped to Saudi Arabia rather than a more central Arabic-speaking region), but the current mappings are reasonable.

  5. Error Handling: The globe could handle network errors more gracefully when the API is unavailable.

Summary

The pull request successfully implements the requested feature and creates an engaging user experience. However, it requires one critical fix (the API endpoint change) and should include additional test coverage for the new functionality. The implementation is creative and technically sound, using a simple but effective approach with Three.js as requested.

Recommendation: Requires changes before merge - specifically fixing the API endpoint issue and adding basic test coverage for the new globe feature.

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.

Show me where the language is on a 3d globe

2 participants