-
Notifications
You must be signed in to change notification settings - Fork 249
feat: detect browser dialect #2485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
elijah-potter
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR is close. There are some irrelevant changes that seem to have crept in without explanation. Overall, I'm a big fan.
| } | ||
|
|
||
| /** Detect English dialect from browser language settings */ | ||
| function detectBrowserDialect(): Dialect { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This logic seems sound. I'd prefer it in its own file with some tests attached.
| }); | ||
|
|
||
| let linter: LocalLinter; | ||
| let linterReady = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is redundant. If the linter is null, we can assume it is not ready. This variable doesn't provide additional information and can become inconsistent with the type system if used incorrectly.
| } | ||
|
|
||
| function initializeLinter(dialect: Dialect) { | ||
| async function initializeLinter(dialect: Dialect) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not need to be modified for this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function does not need to be modified for this PR.
I think the bits you're questioning are from the second commit where we (agent and I) were trying to track down the build problems. So they might be "clutching at straws" before we figured out that the problems were in the WASM/thesaurus stuff and not in the new feature we were working on.
Thanks for casting a more expert eye over it!
|
|
||
| // If user hasn't set a dialect, try to detect from browser language | ||
| if (resp.dialect === undefined) { | ||
| return detectBrowserDialect(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perfect.
|
Sorry @elijah-potter I don't know what changed but when I try to pick up where I left off in this branch I get errors that are outside my wheelhouse and the LLM doesn't give me confidence... |
Issues
N/A
Description
The Harper browser extension always starts in US English mode on Chrome and Safari. I made sure I had my OS and browser settings all on Australian English and no difference.
This is a 99% agent-coded fix to detect the browser language region (dialect) setting.
I've looked at the code but so far I haven't been able to run the tests. Hence just a draft PR for now.
Demo
How Has This Been Tested?
Manually only so far.
Checklist