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 support for non-english zendesk #2207

Merged
merged 4 commits into from
Jun 7, 2023

Conversation

davidlienhard
Copy link
Contributor

Please remember the Contributing Guidelines ❤️

🌟 What does this PR do?

We are using the Zendesk UI from 2021. The Toggl integration is working, but only fetching the ticket number and not the name. But the title would be really helpful for us.
Currently Toggl is searching for an element with aria-label Subject. But this does not exist in the German version of Zendesk, as is named Betreff.
This PR fetches the language of Zendesk from the <html> node, reads the correct Label-Name from the translation array (which can be extended for other languages) and the reads the title.

🐛 Recommendations for testing

You would need to test the Toggle integration in Zendesk with english and german language. Toggle schould add the ticket id and subject for tracking.

📝 Links to relevant issues or information

name of the title element seems to be translated in to the language zendesk is using
so we chck the language of zendesk and fetch the element in the localized name
Copy link
Contributor

@nunofmn nunofmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @davidlienhard! 👋
Thank you for submitting a PR fixing the Zendesk integration. 🙌

Your solution is working fine, but I think we can come up with a more stable selector that is locale independent.
The last time we fixed the selector, we thought that using a selector aiming at the aria-label would guarantee a stable selector, that would resist any potential UI changes.
But we forgot to take into consideration that the aria-label value changes according to the user's locale. 😓

I added a comment suggesting a change to the selector, that makes is locale independent.
If you are able to make the change, I'm happy to re-review and then approve it! 🙌

Thank you for your contribution!

Comment on lines 23 to 31
const language = document.querySelector("html").lang || 'en'

const translations = {
'en': 'Subject',
'de': "Betreff"
}

const elementName = translations[language] || 'Subject'
const input = elem.querySelector('input[aria-label='+elementName+']');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I said in the PR's review comment, this should be a better locale independent selector.
It uses the data-test-id attribute that usually it's pretty stable, since it generally is used internally for E2E tests.

Suggested change
const language = document.querySelector("html").lang || 'en'
const translations = {
'en': 'Subject',
'de': "Betreff"
}
const elementName = translations[language] || 'Subject'
const input = elem.querySelector('input[aria-label='+elementName+']');
const input = elem.querySelector('input[data-test-id="omni-header-subject"]')

@davidlienhard
Copy link
Contributor Author

Hi @nunofmn
Thanks for your quick feedback. I just did some tests with data-test-id=omni-header-subject and they have been successful. I cannot tell how stable that selector really is, but I expect it but be as stable as the old one, if not better.

Please take a look and let me know if changes are required.

Best regards, David

Copy link
Contributor

@nunofmn nunofmn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perfect, LGTM! 👌 🚀

Thank you for your contribution @davidlienhard! 🙌

@nunofmn nunofmn changed the title Add support for german zendesk Add support for non-english zendesk Jun 7, 2023
@nunofmn nunofmn merged commit dcfa263 into toggl:master Jun 7, 2023
@davidlienhard
Copy link
Contributor Author

Great, thank you for the merge. Looking forward to the next release!

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