refactor: useLanguagePicker warning and fallback #12208
Conversation
wackerow
requested review from
corwintines,
pettinarip,
minimalsm and
nhsz
as code owners
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
| if (progressData.length === 0) { | ||
| console.warn("Missing translation progress data; check GitHub action") | ||
| } |
There was a problem hiding this comment.
Looking at this one again, I think we can probably keep this error. It should never throw as long as there is translation progress data available... if not, we should let the build break. cc: @pettinarip Curious your thoughts
There was a problem hiding this comment.
I've to disagree on that. This condition is inside a useEffect. It will never be checked at build time/server side. If it is ever checked, then it will be on client side, creating bad ux since it will make the app crash (unless we are catching that error and failing gracefully).
However, I hear you, it would be nice to have that type of check at build time.
And the change to use console.warn is ok I guess but its not doing anything. Shouldn't we return something else here? given that in the next line we are accessing it progressData[0].words.total.
There was a problem hiding this comment.
Okay yeah, that makes sense, thanks
Shouldn't we return something else here? given that in the next line we are accessing it progressData[0].words.total.
I think we should... I can update this to return early in this case, and pass everything but the approvalProgress and wordsApproved. We can also use localeOption (Lang type) as the fallback for the source or target names... this means a language that failed this for whatever reason would just show the language code instead of an empty string.
Will push a patch
| sourceName: sourceName ?? "", | ||
| targetName: targetName ?? "", |
There was a problem hiding this comment.
@pettinarip Seeing your comment here... I went with this approach to avoid using mutable variables, sticking with const... lemme know if you disagree
There was a problem hiding this comment.
Wondering if we should fallback to default locale or something else. Not sure if "' isn't going to break something else in the chain tbh. If not, it ok.
There was a problem hiding this comment.
It wouldn't break anything, but it would just render without a name... I switched this to fallback to its language code.
wackerow
changed the title
There was a problem hiding this comment.
@wackerow left some questions. Maybe there are potential scenarios we are not considering.
| if (progressData.length === 0) { | ||
| console.warn("Missing translation progress data; check GitHub action") | ||
| } |
There was a problem hiding this comment.
I've to disagree on that. This condition is inside a useEffect. It will never be checked at build time/server side. If it is ever checked, then it will be on client side, creating bad ux since it will make the app crash (unless we are catching that error and failing gracefully).
However, I hear you, it would be nice to have that type of check at build time.
And the change to use console.warn is ok I guess but its not doing anything. Shouldn't we return something else here? given that in the next line we are accessing it progressData[0].words.total.
| sourceName: sourceName ?? "", | ||
| targetName: targetName ?? "", |
There was a problem hiding this comment.
Wondering if we should fallback to default locale or something else. Not sure if "' isn't going to break something else in the chain tbh. If not, it ok.
use localeOption (lang code) fallback for source or target names instead of empty string
Description
useLanguagePicker.tsxto remove error throwing when a source or target locale display name is not foundconsole.warninstead, and adds an empty string fallback to ensure values are neverundefinedRelated Issue