Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jibran’s picture

Status: Active » Needs review
FileSize
11.87 KB

I haven't done the renaming part yet.
contextual.toolbar.js has a model attribute

  var contextualToolbar = Drupal.contextualToolbar;
  var model = contextualToolbar.model = new contextualToolbar.Model();

and we are also doing
if (Drupal.contextual && Drupal.contextual.collection) {
So I don't think that we can move Drupal.contextualToolbar.model to Drupal.contextual.ToolbarModel because if we want to move Drupal.contextualToolbar.VisualView to Drupal.contextual.ToolbarVisualView then we have to change

/**
 * Model and View definitions.
 */
Drupal.contextualToolbar = {
  // The Drupal.contextualToolbar.Model instance.
  model: null
};

to

/**
 * Model and View definitions.
 */
Drupal.contextualToolbar = {
  // The Drupal.contextual.ToolbarModel instance.
  toolbarModel: null
};

which is not right.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

Umm that's fair enough. Good for me, thanks !

We'll see about the renaming once we get around documenting that stuff.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 1: d8.split-contextual.toolbar.js_.2162837-1.patch, failed testing.

jibran’s picture

Status: Needs work » Needs review
FileSize
13.03 KB
1.16 KB

Forgot to add a file.

nod_’s picture

Status: Needs review » Reviewed & tested by the community

All good, thanks.

xjm’s picture

nod_’s picture

The last submitted patch, 4: d8.split-contextual.toolbar.js_.2162837-4.patch, failed testing.

nod_’s picture

Status: Reviewed & tested by the community » Needs work
jibran’s picture

Status: Needs work » Needs review
FileSize
16.97 KB

Reroll

Wim Leers’s picture

Status: Needs review » Postponed

This patch conflicts with #2162409: Split up contextual.js. Let's get that one in first.

webchick’s picture

Status: Postponed » Needs work

That one's in, this will need a re-roll.

jibran’s picture

We can't split it as it is. First we have to change the Model and Views name because we already have AuralView.js for Drupal.contextual.AuralView so we have to rename the Drupal.contextualToolbar.AuralView.

Wim Leers’s picture

Assigned: Unassigned » Wim Leers
Status: Needs work » Needs review
Issue tags: +Spark, +sprint
FileSize
18.36 KB

Yep. Here's a patch that does all that.

jibran’s picture

Nice approach. And thanks for Hijeacking my patch :P :D. I'll let @nod_ RTBC it.

Wim Leers’s picture

Sorry! I really needed needed a break from the huge patches I've been working on. Glad you like it!

jessebeach’s picture

Status: Needs review » Reviewed & tested by the community

The patch still applies.

I manually tested and found no regressions. This patch just pushes some code around into separate files; it does not alter the existing behavior.

Looks good to go.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: split_contextual_toolbar_js-2162837-14.patch, failed testing.

Wim Leers’s picture

Testbot is broken ATM, see #2248217: Update Drush to newer SHA1.

jibran’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Back to RTBC as per #17.

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 14: split_contextual_toolbar_js-2162837-14.patch, failed testing.

jessebeach’s picture

Status: Needs work » Needs review
Wim Leers’s picture

Status: Needs review » Reviewed & tested by the community

Green again, back to RTBC again, again as per #17.

webchick’s picture

Status: Reviewed & tested by the community » Fixed

Committed and pushed to 8.x. Thanks!

  • Commit 4ebc622 on 8.x by webchick:
    Issue #2162837 by Wim Leers, jibran | nod_: Split up contextual.toolbar....
Wim Leers’s picture

Issue tags: -sprint

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.