1. The color schemes in Bartik should be ordered alphabetically, like in Garland.
2. The colors in Bartik should use lowercase letters, like Garland does.
3. Colors like #fffeff are sloppy and should be changed to #ffffff.
4. The array keys for the schemes need to be all lowercase and underscores, like Garland.
5. Bartik needs more color schemes to choose from, like Garland has.

The attached patch should fix the aforementioned problems.

CommentFileSizeAuthor
#15 864252-fix-color-broken-test-d7.patch845 bytesandypost
#7 bartik locks.png9.87 KBdcrocks
#7 garland locks.png9.14 KBdcrocks
#4 bartik_color_02.patch2.89 KBcwgordon7
#2 bartik_colors_clouds.png34.69 KBcwgordon7
#2 bartik_colors_lavender.png34.22 KBcwgordon7
#2 bartik_colors_midnight.png32.81 KBcwgordon7
#2 bartik_colors_peach.png34.98 KBcwgordon7
#2 bartik_colors_watermelon.png34.61 KBcwgordon7
bartik_color.patch4.76 KBcwgordon7

Comments

Bojhan’s picture

Bojhan commented

5. Is a bit of weird one in the crowd here, can you show what you did to fix that?

cwgordon7’s picture

cwgordon7 commented
StatusFileSize
new bartik_colors_watermelon.png34.61 KB
new bartik_colors_peach.png34.98 KB
new bartik_colors_midnight.png32.81 KB
new bartik_colors_lavender.png34.22 KB
new bartik_colors_clouds.png34.69 KB

I added several new color schemes. There aren't really any well-established criteria for what types of color schemes to present to the user, so I added a few that looked cool to me (these are by all means up for review). The schemes I added were "Clouds", "Lavender", "Midnight", "Peach", and "Watermelon".

Screenshots:

Clouds: Only local images are allowed. Lavender: Only local images are allowed.
Midnight: Only local images are allowed. Peach: Only local images are allowed.
Watermelon: Only local images are allowed.  
Bojhan’s picture

Bojhan commented

I think its better if you take those out and remain the patch for addressing the first four issues, which should be fixed asap. To avoid broading the scope of this issue to much, especially as I think introducing those kind of color schemes is a drastic deviation of the care and reviews that went into creating the others.

cwgordon7’s picture

cwgordon7 commented
StatusFileSize
new bartik_color_02.patch2.89 KB

Sure, removed the new color schemes and created a new patch that addresses only the first four issues listed.

Status: Needs review » Needs work

The last submitted patch, bartik_color_02.patch, failed testing.

gdd’s picture

gdd
he/him
Primary language English
Location Portland, OR
commented
Status: Needs work » Needs review

#4: bartik_color_02.patch queued for re-testing.

dcrocks’s picture

dcrocks commented
StatusFileSize
new garland locks.png9.14 KB
new bartik locks.png9.87 KB

If everything is to be like Garland, why not
#6 put lock icons into the same position as Garland.

Sorry, this change to bartik a few months back still bugs me as unnecessary.

cwgordon7’s picture

cwgordon7 commented

I tend to agree, dcrocks, but as Bojhan already pointed out, it's better to keep issues focused and to split out other concerns into different issues. Please create another issue for changing the locks, and post another comment to this issue letting us know where to find that issue.

dcrocks’s picture

dcrocks commented

Understand. This was discussed in http://drupal.org/node/783574#comment-3187288 , where it was stated that bartik didn't want to look like garland, so it was better to change it. I commented here because of the difference in philosophy. That issue still isn't closed but inactive for several weeks.
The lock issue isn't that important but it seems there should be some consistency in the philosophy of bartik's evolution and/or divergence from garland for those who want to contribute to bartik.

jensimmons’s picture

jensimmons commented
Title: Cleanup and improvement of Bartik color schemes » Code cleanup for Bartik color module implementation
Status: Needs review » Needs work
Issue tags: +color module

Only local images are allowed.

This issue should to be broken up into separate issues. http://www.webchick.net/please-stop-eating-baby-kittens

The code cleanup stuff is easier to fix — let's do it here.

Proposing design changes to the colors schemes should be a separate, stand-alone issue.

Discussion of the color module interface for Bartik is yet a third issue — one that already exists at #783574: Clean up color module settings page

Also, about this:

3. Colors like #fffeff are sloppy and should be changed to #ffffff.

No — that's not sloppy. It's that way on purpose. Color module works by finding all the instances of a color and replacing it. If I want all the link colors to be changed at the same time, I make it work by starting with them all as the same color. If I want the color of the links to be altered separately from the color of something else, I make they be separate by giving them separate starting numbers. If I give them the same number, color module will not work properly — at all. Months of thought and effort went into using the D5 color picker in a whole new way for D7. We used very close, but different, hex numbers on purpose, in order to get the thing to work.

Do not change those colors.

You aren't the only one bothered by this use of almost-the-same-color, but there's no way around it for D7. Anyone interested in changing this, help out with rewriting color module for D8.

Meanwhile, the proposed patch needs to be redone without the changes to the color numbers.

And it should be reviewed by smerrill before going anywhere.

cwgordon7’s picture

cwgordon7 commented

jensimmons, I was referring to the final colors, not the original colors. I understand why in the default scheme colors like #fffeff are necessary for replacement purposes. But for the other color schemes, why is #fffeff and like necessary? Am I missing something here?

jensimmons’s picture

jensimmons commented
Status: Needs work » Needs review

Talking to cwgordon7 in IRC tonight, it seems that my sense that all the colors in all the non-default schemes must be different from each other in order to get color module to work — that that understanding of mine might be wrong. And that his patch is a good thing and will work.

So I'm setting this back to needs review. I'd love it if smerrill can test it out, as well as other people.

tim.plunkett’s picture

tim.plunkett
he/him
Primary language English
Location Philadelphia
commented
Status: Needs review » Reviewed & tested by the community

Looks good to me.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: Code cleanup for Bartik color module implementation » [HEAD testing broken] Code cleanup for Bartik color module implementation
Priority: Normal » Critical
Status: Fixed » Needs review
StatusFileSize
new 864252-fix-color-broken-test-d7.patch845 bytes

HEAD testing broken. http://qa.drupal.org/pifr/test/33
Because http://drupal.org/cvs?commit=405836 commited change lowercase keys for schemes

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Status: Needs review » Reviewed & tested by the community
amanada’s picture

amanada commented

bartik_color.patch queued for re-testing.

dries’s picture

dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

andypost’s picture

andypost
he/him
Primary language Russian
commented
Title: [HEAD testing broken] Code cleanup for Bartik color module implementation » Code cleanup for Bartik color module implementation

Fixed title

Status: Fixed » Closed (fixed)
Issue tags: -color module

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

Issue tags: +color module

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.