Comments

andriyun’s picture

Assigned: Unassigned » andriyun
Status: Active » Needs review
FileSize
2.75 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 77,586 pass(es). View
andypost’s picture

Assigned: andriyun » nod_
Issue summary: View changes
Issue tags: +JavaScript

Looks great! Needs JS approval, +1 RTBC

BarisW’s picture

JS looks good to me too, but let's nod_ have a look as well. +1

nod_’s picture

Not sure if this.options = options is a backbone way of doing things.

If someone can confirm it, +1 from me.

nod_’s picture

Assigned: nod_ » Unassigned
dealancer’s picture

I would update a screenshot, as it is does not match to what have been implemented.

+      this.options = options;

This is done automatically in in Backbone 1.1, see http://backbonejs.org/#changelog

+        .attr('title', Drupal.t(
+          '@action contextual links', {
+            '@action': (this.model.get('isViewing')) ?
+              this.options.strings.show : this.options.strings.hide
+            }
+          )

That is a bit hard for understanding, would't be simpler to do something like this?

+        .attr('title', this.model.get('isViewing') ? this.options.strings.showLinks : this.options.strings.hideLinks)

Then,

-      // Update the view of the trigger.
-      this.$el.find('.trigger')
-        .text(Drupal.t('@action @title configuration options', {
+      var buttonText = Drupal.t('@action @title configuration options', {
           '@action': (!isOpen) ? this.options.strings.open : this.options.strings.close,
           '@title': this.model.get('title')
-        }))
-        .attr('aria-pressed', isOpen);
+        });
+
+      // Update the view of the trigger.
+      this.$el.find('.trigger')
+        .text(buttonText)
+        .attr({
+          'aria-pressed': isOpen,
+          'title': buttonText
+        });
     }

Looks like it is a fix for the views module (core/modules/contextual/js/views/AuralView.js). Could you tell what does this fix and where exactly? May be attach a screenshot and update ticket description if required.

Everything else looks good!

Prashant.c’s picture

Patch #1 applying succesfully and also displaying tooltip but its showing message as "Show contextual links" instead of "Show/hide contextual links" as shown in the https://www.drupal.org/files/issues/Screenshot%20from%202014-09-24%2000-....

droplet’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
drupaldrop’s picture

Issue tags: -Needs reroll
FileSize
9.76 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 95,170 pass(es). View

Patch is rerolled - got a conflict and reuploaded the patch after fix . find details below

First, rewinding head to replay your work on top of it...
Applying: Applying patch from issue 2343777 comment 9187899
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
Auto-merging core/modules/contextual/js/toolbar/views/VisualView.js
CONFLICT (content): Merge conflict in core/modules/contextual/js/toolbar/views/VisualView.js
Failed to merge in the changes.
Patch failed at 0001 Applying patch from issue 2343777 comment 918789

Patch is clean now..

drupaldrop’s picture

Status: Needs work » Needs review
droplet’s picture

Status: Needs review » Needs work

wrong patch ? We should not just let GIT happy only while rerolling a patch :)

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

sudhanshug’s picture

Rerolled the patch according to suggestions in #6.

Status: Needs review » Needs work
sudhanshug’s picture

Sorry for the patch in #15. Corrected the patch.

Status: Needs review » Needs work
sudhanshug’s picture

Seems like I missed indentation. :/

Status: Needs review » Needs work
sudhanshug’s picture

Not a good day for me at all. :(((
Paches in #15 and #17 are same.
Here is the tested patch.
Apologies....

Status: Needs review » Needs work