Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Jeff Burnz’s picture

I would punt for simply removing this option from core, I'm not really convinced anyone knows what this is for, how it works or even uses it. Its not really useful when building colorable themes.

Bojhan’s picture

Status: Active » Needs work

I would agree, can we have a patch that does this?

dcmistry’s picture

Being cautious about this, but +1 remove it!

Jeff Burnz’s picture

Yes I agree about being cautious, I think we should leave this open for a while and see what comes, its a bit of work to get this out of color module, however it does actually do something quite sophisticated - shifting the colors relative to each other (that's the best way I can describe it), however I get the distinct feeling this was something designed for Garland and does not have wide application especially for themes that don't work/look good as a monochromatic design.

Like to hear from some other people who actually build and/or use colorable themes. I've built about 20 or so over the years and frankly I have have never used this feature or bothered to take it into account.

mrfelton’s picture

Status: Needs work » Needs review
FileSize
3.52 KB

After discussing this with Bojhan, we both came to the conclusion that the feature is simply a little bizarre, confusing, and unpredictable. Decided to remove the feature to remove the confusion. Attached patch removes the feature altogether.

Status: Needs review » Needs work

The last submitted patch, 1164796.5-remove-lock-icon-from-color-module.patch, failed testing.

aspilicious’s picture

You should remove everything about hooks to. (those are the lines connecting the lock icon to the colors it links)

mrfelton’s picture

Status: Needs work » Needs review
FileSize
7.65 KB

How about this one...

aspilicious’s picture

Status: Needs review » Needs work
+++ b/modules/color/color.jsundefined
@@ -192,39 +178,11 @@ Drupal.behaviors.color = {
       // Add hook.
       var hook = $('<div class="hook"></div>');
       $(this).after(hook);
       ....

Stil some hook stuff left.
But you're getting closer.

5 days to next Drupal core point release.

mrfelton’s picture

Status: Needs work » Needs review

@aspilicious - look at the patch again. Line 233. That part was removed in the second git commit.

aspilicious’s picture

Can you make this one patch please? It's easier for people like me with a gui editor. Else I can't review it this week.

mrfelton’s picture

Well, I'm certainly learning more about working with git! Here you go.

aspilicious’s picture

Code wise this looks ok to me. Just need some manual testing and a second code review and this is good to go :)

mpearrow’s picture

Code also looks good to me - is there a playbook for doing manual testing?

aspilicious’s picture

Before after screenshots and playing around with the color stuff to see if everything still works.

dcmistry’s picture

Good to know we are getting rid of this :)

Bojhan’s picture

@mpearrow Did you get around to testing this? I can do it too, if needed.

tim.plunkett’s picture

Subscribe, might have time for screenshots this week.

mpearrow’s picture

I did indeed get to test, and it all worked fine.

tim.plunkett’s picture

Status: Needs review » Reviewed & tested by the community

The patch is good to go from the code side, and in IRC, Bojhan said it doesn't really need screenshots.

Awesome work mrfelton!

yoroy’s picture

I think this could use a little summary on *what* is getting removed here. I never use color module settings and I'm not clear what it is supposed to do. Which is consistent with prior reports yes, so do remove it but so far this issue doesn't explain the actual functionality we're dropping here.

As per #4 above:

it does actually do something quite sophisticated - shifting the colors relative to each other (that's the best way I can describe it), however I get the distinct feeling this was something designed for Garland and does not have wide application especially for themes that don't work/look good as a monochromatic design.

Ah yes indeed. It allows you to lock colors to eachother so that changing a single color automatically updates all locked colors to maintain their relative offset/difference. I agree with Jeff that although quite sophisticated it only really works in monochromatic color palettes.

Screenshot before:

Screenshot after:

RTBC it is.

Dries’s picture

Personally, I thought it was a pretty neat/handy feature. While it is a bit confusing at first, I was pretty easy to figure out (learn) how it works. I'm OK with removing it though. Let's see if more people have thoughts before I commit this.

Bojhan’s picture

@Dries Sorry, but this has to go. Although it could be a neat/handy feature, no one understands it (even the 6 observers during the test where, wtf does it do?). But also as yoroy explained it only works well for themes with monochromatic color schemes.

EDIT: Also regarding data, by no means quantitative. But given that it wasn't part of the test plan, we found at least one participant who didn't understand it http://www.archive.org/download/Drupal7UmnUsabilityStudy/Disk3_fullfixed... at 13:05

BTMash’s picture

I like the change. Personally, the lock didn't make a whole lot of sense to me. Having to unlock to change a color feels unnecessary. Also as mentioned above, it doesn't seem like a whole lot of themes take advantage of what the lock can do.

james.elliott’s picture

This feature provides the powerful ability to change the color of gradients or color pairs easily. It would be detrimental to the usefulness of the color module to remove the ability to pair the colors of a gradient.

I would suggest the following as an alternative to removing the functionality.

  • Change the lock icon into something more symbolic of a link. The lock icon is a mixed metaphor. The colors aren't being locked. They are being linked to each other.
  • Require that themes specify which sets of colors can be linked, if any. This allows a theme with color enabled gradients to allow linking of the gradient's colors but doesn't force all themes to have the color link confusion in their settings UI.

My proposal removes the UI confusion from themes that do not implement color linking, and allows a better method of linking colors for panchromatic themes. Perhaps we could improve this feature instead of removing it.

EDIT: @BTMash That's not what the lock does. It "locks" the colors distance in relation to each other. It does not stop you from changing them. In truth, it links them together.

David_Rothstein’s picture

Status: Reviewed & tested by the community » Needs review

Right, during usability testing discussions (and as described at the very top of this issue) we identified "Change icon from a lock to a link" as one way to fix this, not just removing it from core.

The link icon is commonly used for this kind of feature in graphical editing programs, as far as I've seen. I don't know if we have any evidence that the underlying feature itself is confusing, even though the lock icon definitely is.

James's proposal seems like a good compromise at first glance.

Bojhan’s picture

@james you mean like a chain? I doubt that we will find a interaction that clearly communicates what this functionality tries to do, but I wish you good luck. I don't think this functionality has any wide application, nor does it do a great job at harmonizing colors in a lot of cases.

However I don't really care about this feature, so I am unsubscribed.

David_Rothstein’s picture

james.elliott’s picture

FileSize
18.7 KB

The icons in #28 are a good option.

For reference, the attached is how Adobe Photoshop handles linking layers.

james.elliott’s picture

FileSize
40.38 KB

Attached is a quick mockup of what my proposal might look like

thedavidmeister’s picture

Status: Needs review » Needs work
Issue tags: +Usability, +UMN 2011

The last submitted patch, 1164796.12-remove-lock-icon-from-color-module.patch, failed testing.

markhalliwell’s picture

I think we can/should keep these. However I think there should clearer constraint on when they should be used (ie: when changing gradients). In reality, I really could be persuaded either way. If we take them out, then patch in #12 needs a reroll. If we keep them, then it needs to be refactored to have better constraints on when two colors can be linked (which btw, I think the link and unlink icons make SOOO much more sense than a "lock").

markhalliwell’s picture

Issue summary: View changes
Status: Needs work » Closed (duplicate)
Related issues: +#2268955: Deprecate farbtastic library

Seeing how there is general consensus (in IRC, issues, refactor) for removing Farbastic entirely, this "feature" will inherently be removed anyway (since it relies on Farbastic's HSL method to "lock/link" colors).

bikeman01’s picture

What happened with this? The padlocks are still there.