Can you please remove the styling that you have for your output? It would be a kindness to those who have to override your CSS in order to make your output go with our sites. It's great if you want to add classes and IDs, but it's a pain for those of us who have to override the colors - especially link colors. It's unnecessary. If the site has a theme, it's got its own link colors. Please take it out. You're just creating unnecessary work - like ie6.

CommentFileSizeAuthor
#12 twitter-pull-873480-12.patch547 bytesstar-szr

Comments

kmonty’s picture

+1

there is no reason for this module to make twitter pull links color: blue;. No theme uses color: blue; as its default link color... just let people inherent their default link colors.

irakli’s picture

Status: Active » Closed (works as designed)

I am kinder to themers. All (I mean ALL) styles in the css file attached by default are for the descendants of div.tweets-pulled-listing element. When you create your TPL file, change the CSS classname on the main div from "tweet-pulled-listing" to something else and your template won't pick-up any of the default styles.

kmonty’s picture

Status: Closed (works as designed) » Active

That isn't how modules should be constructed to work (notice why no other module requires you to override the default tpl?). Do you think anyone wants their twitter links to be color: blue;

Let twitter_pull inherent theme defaults, not define their own defaults.

irakli’s picture

Title: Can you be kinder to themers please? » Inherit link colors from active theme.

OK, let's not make blanket statements.

  • Twitter Pull does not REQUIRE you override default tpl. You only need to override it, if you do not like the default look-n-feel and want to change it. That's the whole point of theming. You don't like - you change. There's nothing unusual about it.
  • There're plenty of modules who provide default theming and expect you to override. Views is the most known example.
  • Twitter Pull needs some default CSS so it does not look broken out of the box.

All these said, and re-reading the comments, looks like blue link color is the cause of most anger here. I don't have any problem killing that one. I am going to have to edit the title of the issue, though, because current title does not reflect what needs to be done here. There's important difference between not providing any style and not liking that the default link is blue.

Default styles (layout etc.) stay, link color goes away and will be inherited from the theme.

Thank you.

kmonty’s picture

This also should be pulled out for the same reason:

	div.tweets-pulled-listing .tweet-time a {
24	  color: silver;
25	}
Marko B’s picture

I agree, link color and stayling altogether should be inherited from active theme, there is no reason to be otherwise in most cases.

mstrelan’s picture

+1 on the removal of blue links. As for other modules, I cannot think of one that actually sets the color of an element, unless it is to a neutral grey sort of color.

I'm actually not particularly worried about the tweet-time being silver. The reason I say this is that in a large number of scenarios it is very likely that the tweet-time link won't look like a link, and so themers will have to specifically target that link anyway if they don't like the default color. Having said this, to cover the other crowd who want it to look like a link well then it should be unstyled by the module.

mrfelton’s picture

Definitely think the link should not be forced into blue. I can see why you may want to float the image to the left in the default styles, but I can't see any reason why any other style properties should be set. Definitely not the link color, and probably not the color of the date. Bold on the author name, ok maybe. Colors should be inherited from the theme, otherwise 99 times out of 10, the twitter feed block just looks strage with different colored links to everything else on the page.

One other thing, although probably out of scope for this ticket - you probably shouldn't use hook_init() load the styles. They are being loaded for every single page of the site. It would be better to only load the styles when a twitter feed is actually being displayed.

mstrelan’s picture

One other thing, although probably out of scope for this ticket - you probably shouldn't use hook_init() load the styles. They are being loaded for every single page of the site. It would be better to only load the styles when a twitter feed is actually being displayed.

Loading CSS in hook_init() helps to minimise the number of aggregated CSS files are generated. But if CSS aggregation is disabled then it increases the number of CSS files loaded by the browser. In my opinion modules should check if preprocess_css is TRUE and if so load CSS in hook_init() otherwise on demand. I posted my thoughts on this against the Date module #369159: Only add css when required.

Probably the easiest way would be to have a function MYMODULE_load_css() with a static boolean variable $loaded. In hook_init() check if preprocess_css is TRUE and only if it is TRUE call MYMODULE_load_css(). Then every time you need the CSS on demand call MYMODULE_load_css(). That function will check if its already loaded, and if it is then it will do nothing.

star-szr’s picture

Subscribe.

jurriaanroelofs’s picture

a module should _never_ add styling to it's output.
structure and layout, yes if you must.
colors, borders, font-weights, never.

+1 for removal.

star-szr’s picture

Status: Active » Needs review
StatusFileSize
new547 bytes

Here's a patch against 7.x-1.x-dev (applies to 7.x-1.0-rc1, 6.x-1.x-dev, 6.x-1.2 as well). The patch removes all the colors from the stylesheet. Hopefully this very minor change can get committed and we can all move on with our lives.

artfulrobot’s picture

+1 no blue links (sympathies with author who points out that these styles are only for use with the default theme, but event then, the default theme should integrate nicer and not set blue).

+1 only load styles when needed. As the author points out, these styles are for the default theme only. If you're not using the default theme, this is unnecessary bloat whether it's aggregated or not.

(grateful to author for writing and sharing great module!)

mortendk’s picture

+1 for killing of the link colour - was really not a nice surprise when that showed up
atm im killing direcly all style instead from in otherways a good module with the foad

jec006’s picture

Status: Needs review » Patch (to be ported)

Committed here to 7: http://drupalcode.org/project/twitter_pull.git/commit/c8993e8

Thanks guys, especially Cottser

star-szr’s picture

Thanks @jec006! Is the patch (to be ported) for D6? The patch I posted should apply cleanly to 6.x-1.x as well since the stylesheets were the same.