Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Remove this feature from core.
Change icon from a lock to a link.
Comment | File | Size | Author |
---|---|---|---|
#30 | Untitled-1.png | 40.38 KB | james.elliott |
#29 | fire-19.jpg | 18.7 KB | james.elliott |
#12 | 1164796.12-remove-lock-icon-from-color-module.patch | 6.01 KB | mrfelton |
#8 | 1164796.8-remove-lock-icon-from-color-module.patch | 7.65 KB | mrfelton |
#5 | 1164796.5-remove-lock-icon-from-color-module.patch | 3.52 KB | mrfelton |
Comments
Comment #1
Jeff Burnz CreditAttribution: Jeff Burnz commentedI 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.
Comment #2
Bojhan CreditAttribution: Bojhan commentedI would agree, can we have a patch that does this?
Comment #3
dcmistry CreditAttribution: dcmistry commentedBeing cautious about this, but +1 remove it!
Comment #4
Jeff Burnz CreditAttribution: Jeff Burnz commentedYes 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.
Comment #5
mrfelton CreditAttribution: mrfelton commentedAfter 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.
Comment #7
aspilicious CreditAttribution: aspilicious commentedYou should remove everything about hooks to. (those are the lines connecting the lock icon to the colors it links)
Comment #8
mrfelton CreditAttribution: mrfelton commentedHow about this one...
Comment #9
aspilicious CreditAttribution: aspilicious commentedStil some hook stuff left.
But you're getting closer.
5 days to next Drupal core point release.
Comment #10
mrfelton CreditAttribution: mrfelton commented@aspilicious - look at the patch again. Line 233. That part was removed in the second git commit.
Comment #11
aspilicious CreditAttribution: aspilicious commentedCan 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.
Comment #12
mrfelton CreditAttribution: mrfelton commentedWell, I'm certainly learning more about working with git! Here you go.
Comment #13
aspilicious CreditAttribution: aspilicious commentedCode wise this looks ok to me. Just need some manual testing and a second code review and this is good to go :)
Comment #14
mpearrow CreditAttribution: mpearrow commentedCode also looks good to me - is there a playbook for doing manual testing?
Comment #15
aspilicious CreditAttribution: aspilicious commentedBefore after screenshots and playing around with the color stuff to see if everything still works.
Comment #16
dcmistry CreditAttribution: dcmistry commentedGood to know we are getting rid of this :)
Comment #17
Bojhan CreditAttribution: Bojhan commented@mpearrow Did you get around to testing this? I can do it too, if needed.
Comment #18
tim.plunkettSubscribe, might have time for screenshots this week.
Comment #19
mpearrow CreditAttribution: mpearrow commentedI did indeed get to test, and it all worked fine.
Comment #20
tim.plunkettThe patch is good to go from the code side, and in IRC, Bojhan said it doesn't really need screenshots.
Awesome work mrfelton!
Comment #21
yoroy CreditAttribution: yoroy commentedI 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:
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.
Comment #22
Dries CreditAttribution: Dries commentedPersonally, 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.
Comment #23
Bojhan CreditAttribution: Bojhan commented@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
Comment #24
BTMash CreditAttribution: BTMash commentedI 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.
Comment #25
james.elliott CreditAttribution: james.elliott commentedThis 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.
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.
Comment #26
David_Rothstein CreditAttribution: David_Rothstein commentedRight, 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.
Comment #27
Bojhan CreditAttribution: Bojhan commented@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.
Comment #28
David_Rothstein CreditAttribution: David_Rothstein commentedPossible icons (borrowed from Gimp - I assume the icons are GPL but haven't actually checked):
http://git.gnome.org/browse/gimp/plain/themes/Default/images/stock-vchai...
http://git.gnome.org/browse/gimp/plain/themes/Default/images/stock-vchai...
Comment #29
james.elliott CreditAttribution: james.elliott commentedThe icons in #28 are a good option.
For reference, the attached is how Adobe Photoshop handles linking layers.
Comment #30
james.elliott CreditAttribution: james.elliott commentedAttached is a quick mockup of what my proposal might look like
Comment #31
thedavidmeister CreditAttribution: thedavidmeister commented#12: 1164796.12-remove-lock-icon-from-color-module.patch queued for re-testing.
Comment #33
markhalliwellI 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").
Comment #34
markhalliwellSeeing 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).
Comment #35
bikeman01 CreditAttribution: bikeman01 commentedWhat happened with this? The padlocks are still there.