Alter Bartik to run with the modified color module (from #693504) [#749276]

On march 11, dcrocks said at http://drupal.org/node/683026#comment-2706102:

I have attached a diff file with changes to color.inc so that bartik can at least run with the modified color module. I want to be able to have the modified Garland as well as corolla, bartik, and busy running under the same instance of Drupal, so they need to be at the same level of the color module. About all this does for bartik is change the header background and some text colors.
My text editor knows more about diff files than I do for the moment and I haven't figured out the color module yet, so if you see something wrong, please educate me.

Comment	File	Size	Author
#6	bartik-749276.patch	2.54 KB	EvanDonovan
	bartik-color-diff.txt	2.43 KB	jensimmons

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Comment #1

jensimmons CreditAttribution: jensimmons commented 22 March 2010 at 14:22

Does this work? Is this a good idea? Will it hurt Bartik in any way if the proposed changes to color module do not get finished? #749276: Alter Bartik to run with the modified color module (from #693504)

Comment #2

dcrocks CreditAttribution: dcrocks commented 24 March 2010 at 01:16

The changes do work, at least as far as I am able to test. The results are the same with the old and the new color modules. The screen appearance is the same, there are no error messages, and the process of changing color schemes works without a hitch, including choosing 'custom' and setting your own colors, with and without the color wheel. So, the appearance of Bartik is neither different, better, nor worse with the new color module.
But outside of the header and 'a' text colors, nothing is set up to really change in bartik anyways. I looked at it first because I thought the footer colors(which I think offer poor contrast) should change in a manner corresponding to the headers. After looking at more themes, it appears that grayish footers are a common style choice and independent of the rest of the color scheme.
I think that it is actually color.css that needs some work if 'color' is to be more important to the bartik scheme. The color module may need some work. It would be nice if custom schemes could be saved as 'named' choices, instead of hand editing the color.inc file.

Comment #3

jensimmons CreditAttribution: jensimmons commented 29 March 2010 at 00:12

Priority:	Normal	» Minor
Status:	Active	» Postponed

I'm going to set this to postponed until something happens with color module. It's not needed until then.

The desire to be able to colorize more regions in Bartik is the whole reason people worked on improving color module. Anyone who wants to see that happen should help out: #693504

Any other thoughts or concerns about the color of Bartik should be discussed in another issue, not here. (1 issue per issue, please).

Comment #4

jensimmons CreditAttribution: jensimmons commented 29 March 2010 at 00:13

Priority:

Minor

» Normal

I meant to just postpone, not to make minor.

Comment #5

EvanDonovan CreditAttribution: EvanDonovan commented 2 April 2010 at 20:31

ksenzee just re-rolled the patch for changing the color module. I'll try to test dcrocks' patch within this weekend. Still postponed, though, until ksenzee's patch gets in to core.

Comment #6

EvanDonovan CreditAttribution: EvanDonovan commented 2 April 2010 at 21:56

Title:	Alter Bartik to run with the modified color module	» Alter Bartik to run with the modified color module (from #693504)
Status:	Postponed	» Needs review

File	Size
bartik-749276.patch	2.54 KB

I've re-rolled dcrocks' patch for the latest CVS of Bartik.

The following patch was made using cvs diff -up from within the sites/all/themes/bartik directory. Since the change is in a subdirectory, I've had trouble getting it to apply. It applies, however, if you do patch -d color < bartik-749276.patch.

I know that technically this issue is still postponed on #693504: Color.module does not support more than 5 colors and has hard-coded labels, but I wanted a review of it. You'll have to apply ksenzee's patch in the other issue before you can test this one. I don't know what happens if you try it on an unpatched color.module.

Comment #7

dcrocks CreditAttribution: dcrocks commented 3 April 2010 at 04:07

Tried it on latest cvs bartik. Patch failed for me but instead of spending time on what is probably my error, just used the color.inc I had already created. It does come up with the 'custom' label 1st time, instead of 'default', as noted elsewhere, but this was consistent with garland and corolla as well.

On unpatched color module, you should get a bunch of index errors.

Comment #8

EvanDonovan CreditAttribution: EvanDonovan commented 3 April 2010 at 06:28

dcrocks: The patch has to be applied from the bartik directory via patch -d color < bartik-749276.patch.

With the newly updated issue #693504, it shouldn't say Custom 1st time anymore. I am about to test that.

Comment #9

EvanDonovan CreditAttribution: EvanDonovan commented 3 April 2010 at 06:59

It works great with the new patch on #693504: Color.module does not support more than 5 colors and has hard-coded labels. No issues that I could see.

Comment #10

jensimmons CreditAttribution: jensimmons commented 3 April 2010 at 16:03

Status:

Needs review

» Fixed

So #693504: Color.module does not support more than 5 colors and has hard-coded labels was committed last night :) Time for this patch to go in. What a pleasure it is to finally commit this!

Great work everyone who made this happen! Especially Steven Merrill who got the whole thing going.

Now it's time to do other work on Bartik's implementation of color module choices so we can support additional colors. Do that here: #700170: Bartik Color Module Integration I summarized where we are at here: http://drupal.org/node/700170#comment-2802544

Comment #11

EvanDonovan CreditAttribution: EvanDonovan commented 3 April 2010 at 19:40

Excellent! I will take a look at that probably on Monday.

Comment #12

EvanDonovan CreditAttribution: EvanDonovan commented 3 April 2010 at 19:45

Added a few comments to the other issue.

Comment #13

dcrocks CreditAttribution: dcrocks commented 3 April 2010 at 20:19

Using 7.x-dev dated 4/3 with patched color module already included
bartik dated 4/3
corolla beta 4 dated 4/1

The latest dev release specifies the use of 'default theme' for the administrative theme(by default), so accidentally got to see bartik and corolla as admin theme for 1st time. They both have serious problems with the color module portion of the display. It appears all layout is lost and some elements overlay others. Again, I think the problem is lack of definition in preview.css.