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

Add £, €, ₹ to the currency key #21

Closed
wants to merge 1 commit into from
Closed

Add £, €, ₹ to the currency key #21

wants to merge 1 commit into from

Conversation

BlackyHawky
Copy link
Contributor

@BlackyHawky BlackyHawky commented Jul 16, 2023

This PR improves #669 and resolves issue 666.

Keyboard languages of countries that use euro as their currency will have "€" on the associated key; the English language (GB) will have "£"; India will have "₹" and other countries will have "$" by default.

Tested on Samsung Tablet with Android 13 and on Huawei phone with Android 10.

If needed, I'll add screenshots tomorrow.

@Helium314
Copy link
Owner

I very much prefer leveraging the existing currency key logic, see 0d11066.

This doesn't duplicate code and doesn't touch subtype locales and language tags (which may have unexpected side effects).
Further, changes only need to happen in one place, e.g. when adding the Turkish Lira icon, see the todo
https://github.com/Helium314/openboard/blob/ffe7d81ebc435f9998c2c61b8f07a01099b3d284/app/src/main/res/xml/key_styles_currency.xml#L116-L117

Do you think there is anything not achieved by 0d11066?

@BlackyHawky
Copy link
Contributor Author

Nothing to say ; it's much better than what I proposed.

I tested and found no error.

Thanks for showing me another solution.

@Helium314
Copy link
Owner

Alright, then I'll merge my variant.
Thanks for testing (and for indirectly pushing me to work on this 😉)

@Helium314 Helium314 closed this Jul 19, 2023
@BlackyHawky BlackyHawky deleted the Currency_Key branch July 19, 2023 20:13
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