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

UHF-10161: Create leijuke script for Telia ACE chat #772

Merged
merged 12 commits into from
Jul 1, 2024

Conversation

juho-lehmonen
Copy link
Contributor

@juho-lehmonen juho-lehmonen commented Jun 6, 2024

UHF-10161, UHF-9328

What was done

  • UHF-10161: Ports over Telia's load_ace.js implementation to a local Drupal library. Uses a modified version of the Chatleijuke scripts.
  • UHF-9328: Adds cookie descriptions for Telia ACE implementations

How to install

  • Make sure your instance is up and running on latest dev branch. Recommended to use the Palvelukeskus instance.
    • git pull origin dev
    • make fresh
  • Update the Helfi Platform config
    • composer require drupal/helfi_platform_config:dev-UHF-10161-telia-ace-script
  • Update HDBT
    • composer require drupal/hdbt:dev-UHF-10161-telia-ace-script
  • Run drush locale:check && drush locale:update
  • Run make drush-updb drush-cr

How to test

Continuous documentation

  • This feature has been documented/the documentation has been updated

Translations

  • Translations have been added to .po -files and included in this PR

Other PRs

@juho-lehmonen juho-lehmonen changed the title Draft: UHF-10161: Create leijuke script for Telia ACE chat UHF-10161: Create leijuke script for Telia ACE chat Jun 10, 2024
@juho-lehmonen juho-lehmonen marked this pull request as ready for review June 10, 2024 06:19
@juho-lehmonen juho-lehmonen requested a review from Arkkimaagi June 10, 2024 06:19
Copy link
Contributor

@Arkkimaagi Arkkimaagi left a comment

Choose a reason for hiding this comment

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

Hi!

Great review instructions, thanks!

  • I'm occasionally seeing the chat not opening on click. Instead the whole button disappears. Reloading the page helps, but it's kinda bad experience for the user.
  • The chat on my instance is missing chat_title and the chat only has the speech_bubble icon, no text
  • Translations are not working, as in the loading text does not get translated.
  • After opening the chat once, closing it and reloading the page, the chat seems to automatically open in a different state (chevron_down visible). Is this an ACE bug or a bug in how we initialise the chat once cookies are set? The same happens also if I accept all cookies upon entering the site on a fresh browser. Are we sure it will not open connections to a chat person on each pageload?
  • We need to enable linters in this repository, this code does not pass our HDBT linters that might catch problems etc.

All in all, code seems to be mostly of good quality, these issues are still blockers IMO. So close. :)

assets/css/telia_ace.css Outdated Show resolved Hide resolved
assets/js/telia_ace.js Outdated Show resolved Hide resolved
assets/js/telia_ace.js Outdated Show resolved Hide resolved
assets/js/telia_ace.js Show resolved Hide resolved
assets/js/telia_ace.js Outdated Show resolved Hide resolved
@Arkkimaagi
Copy link
Contributor

Oh, and please add the chat buttons and other ui to our mobile menus "hide when mobile menu is open" selector list.
image

@juho-lehmonen
Copy link
Contributor Author

Oh, and please add the chat buttons and other ui to our mobile menus "hide when mobile menu is open" selector list.

Selectors have been added to the HDBT theme and the PR instructions have been updated.

@juho-lehmonen
Copy link
Contributor Author

  • I'm occasionally seeing the chat not opening on click. Instead the whole button disappears. Reloading the page helps, but it's kinda bad experience for the user.
  • The chat on my instance is missing chat_title and the chat only has the speech_bubble icon, no text
  • After opening the chat once, closing it and reloading the page, the chat seems to automatically open in a different state (chevron_down visible). Is this an ACE bug or a bug in how we initialise the chat once cookies are set? The same happens also if I accept all cookies upon entering the site on a fresh browser.

Unfortunately all of these issues come from the Telia ACE code, but we'll report these to Telia after testing the Chat in the correct demo environment. Some of these might be caused by the local domain being whitelisted and others are configuration issues.

  • Are we sure it will not open connections to a chat person on each pageload?

Yes, the user needs to click a button inside the chat window (and enter their name etc) before anything is connected to the system.

  • Translations are not working, as in the loading text does not get translated.

This should be working now, can you try rerunning drush locale:check && drush locale:update; drush cr inside the VM? This should update the UI translations.

  • We need to enable linters in this repository, this code does not pass our HDBT linters that might catch problems etc.

This file should be compliant with the linter rules now. Created a new ticket for linting all scripts in platform config: https://helsinkisolutionoffice.atlassian.net/browse/UHF-10251

@juho-lehmonen juho-lehmonen requested a review from Arkkimaagi June 19, 2024 08:44
@juho-lehmonen juho-lehmonen force-pushed the UHF-10161-telia-ace-script branch from 930e0af to 5541963 Compare June 19, 2024 10:30
@juho-lehmonen juho-lehmonen force-pushed the UHF-10161-telia-ace-script branch from 4a3228f to 796da02 Compare July 1, 2024 10:59
Copy link

sonarqubecloud bot commented Jul 1, 2024

Copy link
Contributor

@Arkkimaagi Arkkimaagi left a comment

Choose a reason for hiding this comment

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

Looks mostly good to me. The translations do not seem to work on my instance, it still keeps saying the loading thing in Englist on a Finnish page.

Sorry it took so long to review. 🌹

I thin k we can Approve this if you double check the language thing on your machine.

@juho-lehmonen juho-lehmonen merged commit 4599881 into main Jul 1, 2024
4 checks passed
@juho-lehmonen juho-lehmonen deleted the UHF-10161-telia-ace-script branch July 1, 2024 13:08
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