Conversation
✅ Deploy Preview for teal-figolla-ef4831 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
❌ Deploy Preview for unique-figolla-0d1392 failed.
|
❌ Deploy Preview for spontaneous-ganache-9d995f failed.
|
❌ Deploy Preview for calm-shortbread-db1ac1 failed.
|
❌ Deploy Preview for gleaming-mermaid-1c37de failed.
|
cboiteux2765
left a comment
There was a problem hiding this comment.
The code functionality mostly looks good, just some small code good practice comments and have some different mock api unit tests that don't result in the same constant output for related words & definitions.
| lemma: string | ||
| } | ||
|
|
||
| export interface CreateCardParams { |
There was a problem hiding this comment.
If you're already using lang & lemma interface combo for Lookup Params you can just use a LookupParams attribute in here.
There was a problem hiding this comment.
Good callout, I'll update CreateCardParams to extend LookupParams
| } | ||
|
|
||
| class ApiClient { | ||
| private baseUrl: string |
There was a problem hiding this comment.
just curious, why did you decide to make these global vars private, as well as the request function?
There was a problem hiding this comment.
I didn't write this file. This is from the mockup that Eswar worked on. But if I were to guess, it's because no other files need these vars.
There was a problem hiding this comment.
nevermind the token should be private for sure, as long as it's encrypted or stored behind .env so this is fine I think
| export const apiClient = new ApiClient(API_BASE_URL) | ||
|
|
||
| // Mock data for development | ||
| export const mockLookup = async (word: string, language: string) => { |
There was a problem hiding this comment.
Instead of a mock lookup that supports all words & languages but has constant definitions, examples, and related words (I know it's just a mock), can you create different "unit tests" with different mock examples across several popular languages and words? Because it might be confusing to see the same output from all different (word, language combos) in the mock and keeping it 1:1 helps us determine if it works for the different inputs
There was a problem hiding this comment.
This method is just a placeholder; It's no longer used, as we're actually making API calls now.
| word: string | ||
| language: string | ||
| definitions: string[] | ||
| examples: { src: string; tgt: string }[] |
There was a problem hiding this comment.
Can you define what you mean by src and tgt examples here with a small comment?
|
|
||
| export default function LookupPage() { | ||
| const [searchQuery, setSearchQuery] = useState('') | ||
| const [selectedLanguage, setSelectedLanguage] = useState('Spanish') |
There was a problem hiding this comment.
Since you don't want this to be hard coded you just set it to Spanish to be able to see the examples right? For the future it could be coded to the user's default language in their region but this is fine for now, just making sure you're aware
There was a problem hiding this comment.
Oops, yes I meant to change this. I like your idea about setting it to the user's default language
|
Out for 2 weeks so the Netlify deployment fix will be delayed but you can opt to ignore whitespace changes so that git diff doesn't show those: git merge -Xignore-space-at-eol to remove them in the merge and git diff -w command to show the non-whitespaced file diffs. |
cboiteux2765
left a comment
There was a problem hiding this comment.
Still some nit comments but I don't see major issues with code functionality so should be good to merge
| className="w-full flex items-center justify-between p-4 bg-gradient-to-r from-terracotta/5 to-ochre/5 rounded-xl border-2 border-dashed border-terracotta/30 hover:border-terracotta/50 transition-all duration-300" | ||
| > | ||
| <span className="font-serif font-bold text-xl text-ink flex items-center gap-2"> | ||
| <span>✍️</span> |
There was a problem hiding this comment.
Very nit change (can be a future addition) but just not sure if the emoji will render on all platforms
| } | ||
|
|
||
| class ApiClient { | ||
| private baseUrl: string |
There was a problem hiding this comment.
nevermind the token should be private for sure, as long as it's encrypted or stored behind .env so this is fine I think

Description
Replaced the hardcoded search results with the real API call results so that it dynamically pulls from DynamoDB
Related issue(s)
Link the issue(s) that this PR addresses.
Type of change
How did you test this?
Checklist
GenAI usage