Skip to content
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

Improve japanese locale #866

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

twinbird
Copy link

@twinbird twinbird commented Jan 9, 2025

Improved the Japanese locale for better naturalness.
I have attached a reference image showing the display of the standard confirm function in Chrome with Japanese settings.
chrome_confirm_message

@tiesont
Copy link
Member

tiesont commented Jan 11, 2025

Would, at a minimum, need to fix the failing test - the test still asserts the old (current) text of the "confirm" button.

If I could get confirmation from a few more native speakers of Japanese, I'd be fine with making this change. I just don't have any knowledge of how correct this change would be.

@twinbird
Copy link
Author

Sorry, the commit did not include the dist directory, so I have fixed it.

@tiesont
Copy link
Member

tiesont commented Jan 12, 2025

Commit shouldn't include /dist - that's all generated when running grunt.

@twinbird
Copy link
Author

It seems that grunt is not being executed before running the tests in GitHub Actions.
The CI error before including the dist directory might have been caused by this.
Is this the expected behavior?
https://github.com/bootboxjs/bootbox/blob/master/.github/workflows/tests.yml#L27

@tiesont
Copy link
Member

tiesont commented Jan 12, 2025

It seems that grunt is not being executed before running the tests in GitHub Actions. The CI error before including the dist directory might have been caused by this. Is this the expected behavior? https://github.com/bootboxjs/bootbox/blob/master/.github/workflows/tests.yml#L27

I will take a look. It's been a while since we set that action up, so it's possible it doesn't match the current build expectations (generating all of the /dist contents is a relatively new update to this repo).

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.

2 participants