Selectors clean-up: bartik/color theme

horrible selectors

tag

JSHint complains

Line 13: callback: function(context, settings, form, farb, height, width) {

'width' is defined but never used.

Line 13: callback: function(context, settings, form, farb, height, width) {

'height' is defined but never used.

Line 13: callback: function(context, settings, form, farb, height, width) {

'farb' is defined but never used.

Also, when I get D8 up and running I can refactor the selectors to be more efficient.

First revision patch. I need to look at where Drupal.color.callback is called to remove the arguments no longer used in this method.

I am corrected the patch naming so it can be tested properly.
EDIT
I actually still did not name it correctly... I shall ask around before uploading another.

proper way to send patch is to put it as Need review and someone else not involved in the patch to put it as reviewed and tested by the community. You can't do that on your own for core patches :)

had a quick look, looks ok :)

Gotcha. Thanks for the tip;

I have reviewed the patch and added some improvements. Also added a interdiff.txt for easy reviewing ;)

#8: 1751436-bartik-color-js-cleanup-8.patch queued for re-testing.

Only change is for the variable declaration so that they follow core standards.

Hey, I'm really sorry that I'm late to the party, but is there a reason that the core standards are this way? Most of the javascript coding world has settled on the single var pattern, ala patch #8. If this convention were used everywhere, it would probably add up to a larger than trivial amount over the wire. We also wouldn't have knowledgable JS developers coming to Drupal and wondering what we're doing. I can't find where it's stated that it should be this way, and why.

Thank you...

Yes you're a few months late to the party. #1483396: [policy, no patch] JavaScript coding standards for variable declarations, look at #16

"Most of the javascript coding world has settled on the single var pattern", nodejs is not the javascript world, it's part of it. As a wikipedia editor would say: "need references".

Also if code style is your thing and you want the doc to be updated, check out #1778828: [policy, no patch] Update JS coding standards.

tagging for my reference (component maintainer now)

Re-rolled!

After patch application, I played with the Bartik color scheme preview for a while, was silent in the Chrome/FFx console.

we want a "var" for each variable here:

      var $preview = $form.find('#preview'),
          $previewBlock = $preview.find('#preview-block'),
          $palette = $form.find('#palette'),
          $colorVals = $palette.find('input[name*=palette]'),
          paletteValues = {},
          gradientStart, gradientEnd;

Add the proposed changes in #20

Let's wait for testbot and @nod_'s opinion but looks RTBC. Thanks @dimaro!

RTBC++

Testing JavaScript using eslint


core/themes/bartik/color/preview.js
  17:6  error  Split 'var' declaration into multiple statements  one-var

✖ 1 problem (1 error, 0 warnings)

eslint test failed

Plus this issue is a normal task so we need to outline how it fits within the allowable Drupal 8 beta criteria. Can someone add Drupal 8 beta phase evaluation template to the issue summary.

Corrected

Beta eval added.

Code lint properly and color preview still works.

The summary suggests this patch has a performance impact; can someone clarify what that is? (Also in the parent issue at #1574470: Selectors clean-up since I imagine that would apply to other child issues as well.)

Also, is there any disruption from this to contrib or custom modules or themes? Any JS BC break?

Perf impact is present but not that important. There is no API change, it's internal bartik refactoring. The color api is defined in the color module, bartik is using it like any other contrib module would.

About the perf, we're reducing our selection scope, instead of selecting elements from the wrapping <form> element, we're selecting elements from inside <div id="preview"> only.

Do we still to update the IS or does this makes it clear enough?

This needs reroll though.

Patch re-rolled

@nod_, I'd suggest updating the summary of #1574470: Selectors clean-up, since whether or not this patch has actual performance impact affects whether it can be accepted during the beta. See https://www.drupal.org/core/beta-changes. In general, internal refactorings are no longer allowed during the beta phase, for both PHP and JavaScript. If the performance impact is negligible, it might be better to postpone these cleanups.

If anyone could help to reroll it. I promise you I will help you review it and get it move forward.

Now, it's only needed some basic caching for selectors,eg

form.find('.color-preview

I couldn't find any more code to cached as I see all selectors are being cached properly. :)

Looking at wrong file ?

http://cgit.drupalcode.org/drupal/tree/core/themes/bartik/color/preview.js

Every lines are needed some work :) Thanks!

I have updated the patch.

Patch #33 is outdated. You can't reroll it directly. Many code is already removed.

@Droplet: I have just checked the Patch #33 with the latest code and I am able to find all the lines in the Patch.

Following line was mismatching, which I have updated:

form.find('.color-preview .color-preview-header').attr('style', "background-color: " + gradient_start + "; background-image: -webkit-gradient(linear, 0% 0%, 0% 100%, from(" + gradient_start + "), to(" + gradient_end + ")); background-image: -moz-linear-gradient(-90deg, " + gradient_start + ", " + gradient_end + ");");

Please correct me if I am wrong :) Thanks!

@sashi.kiran,

You may right :) Sadly, I think Patch #34 / #42 are not so reviewable.

Since it's rarely used in other pages, I think only a simple improves is acceptable.

@droplet Update this issue to run your patch.

#45 is good to go.

We're going for something simple so while #34/#40 work an are an improvement, the color module needs a refactoring and we can improve that when it'll happen, for now we try to change as little as possible.

Ready to ship .. hu ha
RTBC ++

Committed/pushed to 8.1.x, thanks!