Comments

keeganstreet created an issue. See original summary.

keeganstreet’s picture

rli’s picture

Status: Active » Needs review
nick_schuch’s picture

Thanks for the patch keeganstreet!

You will need to modify the "tour.es6.js" instead as per https://www.drupal.org/node/2815083

nick_schuch’s picture

Status: Needs review » Needs work
keeganstreet’s picture

Thanks @nick_schuch, I'm adding a new patch now for 8.4.x which includes the change in the ES6 source file.

mgifford’s picture

Version: 8.2.x-dev » 8.4.x-dev
Status: Needs work » Needs review
Sumit kumar’s picture

Status: Needs review » Reviewed & tested by the community

Hi Apply patch #6 on 8.4.x and its applied successfully.
working fine

larowlan’s picture

Looks good to me

larowlan’s picture

Issue tags: +JavaScript
Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work
+++ b/core/modules/tour/js/tour.es6.js
@@ -155,7 +155,7 @@
-              link: '<a href=\"#close\" class=\"joyride-close-tip\">&times;</a>',
+              link: '<a href=\"#close\" class=\"joyride-close-tip\" aria-label=\"Close\">&times;</a>',

+++ b/core/modules/tour/js/tour.js
@@ -76,7 +76,7 @@
-              link: '<a href=\"#close\" class=\"joyride-close-tip\">&times;</a>',
+              link: '<a href=\"#close\" class=\"joyride-close-tip\" aria-label=\"Close\">&times;</a>',

We need to translate these with Drupal.t()

Dinesh18’s picture

Status: Needs work » Needs review
FileSize
1.18 KB

Here is an updated patch with Drupal.t()

larowlan’s picture

Status: Needs review » Needs work
+++ b/core/modules/tour/js/tour.es6.js
@@ -155,7 +155,7 @@
+              link: Drupal.t('<a href=\"#close\" class=\"joyride-close-tip\">&times;</a>'),

+++ b/core/modules/tour/js/tour.js
@@ -76,7 +76,7 @@
+              link: Drupal.t('<a href=\"#close\" class=\"joyride-close-tip\">&times;</a>'),

We lost the aria-label attributes.

We shouldn't be translating the whole string, just the 'Close' text.

So, probably best to evaluate Drupal.t('Close') into a variable, and then use string concatenation + in JavaScript to use the translated text with the existing one.

Gábor Hojtsy’s picture

$ git grep “t(‘Close’”
core/modules/contextual/js/contextual.es6.js:        close: Drupal.t(‘Close’)
core/modules/contextual/js/contextual.js:      close: Drupal.t(‘Close’)
core/modules/quickedit/js/views/EntityToolbarView.es6.js:              label: Drupal.t(‘Close’),
core/modules/quickedit/js/views/EntityToolbarView.js:          label: Drupal.t(‘Close’),

Looked if we have any prior art with context (because "close" could mean "nearby" as well :D) but apparently not, so we can assume people already translate this as “Finish” not “Nearby”.

Dinesh18’s picture

    toggleTour: function toggleTour() {
      if (this.model.get('isActive')) {
        var $tour = this._getTour();
        this._removeIrrelevantTourItems($tour, this._getDocument());
        var that = this;
		var $close = Drupal.t('Close');
        if ($tour.find('li').length) {
          $tour.joyride({
            autoStart: true,
            postRideCallback: function postRideCallback() {
              that.model.set('isActive', false);
            },

            template: {
              link: '<a href=\"#close\" class=\"joyride-close-tip\" aria-label=\"' . $close . '\">&times;</a>',
              button: '<a href=\"#\" class=\"button button--primary joyride-next-tip\"></a>'
            }
          });
          this.model.set({ isActive: true, activeTour: $tour });
        }
      } else {
        this.model.get('activeTour').joyride('destroy');
        this.model.set({ isActive: false, activeTour: [] });
      }
    },

added as per #13. Is it good so that I can create the patch

larowlan’s picture

Hi @Dinesh18 in Javscript, use + not . to concatenate.

Also, use close, not $close, as its a plain variable - not a jQuery reference

Dinesh18’s picture

Thanks larowlan. Here is an updated patch.

Dinesh18’s picture

Status: Needs work » Needs review
rogierbom’s picture

Issue tags: +sprint
droplet’s picture

Status: Needs review » Needs work
Issue tags: +Needs JS testing

Let's clean up the ugly concatenation with ES6 template literals and drop the var.

Version: 8.4.x-dev » 8.5.x-dev

Drupal 8.4.0-alpha1 will be released the week of July 31, 2017, which means new developments and disruptive changes should now be targeted against the 8.5.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

naiduharish’s picture

Status: Needs work » Needs review
FileSize
1.7 KB

Modified patch by using template literal and const.

Version: 8.5.x-dev » 8.6.x-dev

Drupal 8.5.0-alpha1 will be released the week of January 17, 2018, which means new developments and disruptive changes should now be targeted against the 8.6.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sim_1’s picture

I wasn't able to apply the patch from #22 when I tried. I believe this is because the original files have changed.

I reapplied the changes manually and then tested with a screen reader. The close buttons read correctly when navigating via screen reader and the markup looked correct to me.

I wasn't able to create an interdiff because I couldn't apply the previous patch. The patch itself is unchanged, only the files it changes are different.

idebr’s picture

Assigned: keeganstreet » Unassigned
Status: Needs review » Needs work

This implementation does not pass our eslint coding standards:

/core/modules/tour/js/tour.es6.js
158:21 error Unexpected template string expression no-template-curly-in-string

https://eslint.org/docs/rules/no-template-curly-in-string

drpal’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
587 bytes

- Should be wrapped in backticks to be a valid template string.

idebr’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community
FileSize
270.2 KB
  • Patch adds descriptive text for screenreader
  • Code passes coding standards

Screenshot of DOM for good measure:

Gábor Hojtsy’s picture

Status: Reviewed & tested by the community » Needs work

I was wondering how did "Close" become "close" between 17 and 22, I did not find commentary about it. As per my earlier grep from above, we consistently use uppercase labels for this (as usual in Drupal). This should be a minor update so we can get this land :) (I would not make this require JS testing personally, that would be shooting a sparrow with a cannon as we say in Hungary).

$ git grep "t('Close'"
core/modules/contextual/js/contextual.es6.js:        close: Drupal.t('Close'),
core/modules/contextual/js/contextual.js:      close: Drupal.t('Close')
core/modules/quickedit/js/views/EntityToolbarView.es6.js:              label: Drupal.t('Close'),
core/modules/quickedit/js/views/EntityToolbarView.js:          label: Drupal.t('Close'),

$ git grep "t('close'"
[no results]
drpal’s picture

Status: Needs work » Needs review
FileSize
1.69 KB
587 bytes

- c -> C

drpal’s picture

- c -> C
- for real this time.

drpal’s picture

idebr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: -Needs JS testing

Feedback from #28 has been addressed in the patch in #30, updating status to RTBC.

Removing 'Needs JS testing' per #28

alexpott’s picture

Adding review credits to everyone whose reviews affected the patch.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed 987e147c0d to 8.6.x and d0af739e12 to 8.5.x. Thanks!

Backported to 8.5.x as a normal bugfix that improves accessibility and is low risk.

  • alexpott committed 987e147 on 8.6.x
    Issue #2885583 by drpal, keeganstreet, Dinesh18, naiduharish, sim_1,...

  • alexpott committed d0af739 on 8.5.x
    Issue #2885583 by drpal, keeganstreet, Dinesh18, naiduharish, sim_1,...