Closed (fixed)
Project:
Drupal core
Version:
6.x-dev
Component:
taxonomy.module
Priority:
Normal
Category:
Bug report
Assigned:
Unassigned
Reporter:
Created:
17 Jan 2008 at 00:56 UTC
Updated:
1 Feb 2008 at 16:32 UTC
Jump to comment: Most recent, Most recent file
Comments
Comment #1
Anonymous (not verified) commentedSubmitting a fix to be included in the default themes. This is a theme issue, and not a module issue. The module properly defines a CSS class for the term description and the layout of the description should be left up to the theme itself without having to override the module.
Comment #2
panchoDon't like that patch. While the individual themes may do whatever they like (dotted borders, colors, blinking text... :), we should make sure this is acceptably rendered for all themes.
Therefore I added some margin to taxonomy.css and added it to taxonomy_term_page(). Looks good now, nothing special, just okay.
Please review and RTBC, if it's okay.
Comment #3
webchickNeed a CSS guru to weigh in on what's the best approach. I agree that it's annoying for every single theme to have to implement this. And yet, I hear complaints all the time about core CSS trying to do too much and having to "untheme" it to get things to do what you want.
Comment #4
webchickI checked w/ quicksketch and he confirmed that taxonomy.css is the better approach. So how about we combine the both, and put the margin in taxonomy.css (so all themes get at least *something*) and the borders into the themes' CSS? I like boydjd's approach because it makes it clear that the description is /not/ part of the page content but something different.
Comment #5
quicksketchWebchick beat me to it, but here's a followup :)
I prefer Pancho's patch because module CSS files should provide a reasonable, simple amount of CSS. Without the theme doing any CSS, it should at least not look horrible. That said, I don't mind combining the two patches so that themes also provide some additional CSS.
This is just a re-roll of #2, but the drupal_add_css() is now in the theme_ function for the page, not in the menu callback.
Comment #6
panchoYeah, that makes more sense!
If you want, we can increase the margin-bottom by another 10px. That should be enough.
I don't like that idea having a dotted border around those descriptions in all core themes, a solid border would be only marginally better. The borders look weird and certainly don't fit into the general appearance of either theme.
Take a look at Steven's comment on adding a border (a thick one, but anyway) to the ‹blockquote› tag. In the end, he is right - we shouldn't rush into adding prominent design elements to an existing theme, as that would break its elegance.
I think with some proper margins we are done, less is more, more is less.
Comment #7
kasperjames commentedRTBC? sorry im learning bazaar and terms. dont fallow.
is that "commit" termology? lloll
Comment #8
webchickOk. Since quicksketch trumps me in CSS knowledge (then again, so does my cat...), and since borders/colours/etc. can quickly turn into a bikeshed argument, how about we get #5 committed to address the 'critical' bug here, and then we can always discuss 'make prettier' in another issue.
Tested, works. Confirms to coding standards. This is the minorest re-roll in the entire world that just removes the newline before the new rule for consistency with the others in taxonomy.css. Please don't credit me for the fix.
RTBC.
Sorry, boydjd. :\ Let's work on the nicer looking ones in your patch after this one is committed. :)
Comment #9
webchickAnd to make it easier to review, here are before and after screenshots.
Comment #10
kasperjames commentedso? would applying ".css" slow down performance vs. layout/design?
or would applying it in the "theme" with basic functions and codes be the way to go?
i assume this is a functionality issue: site load vs. over inhanced design layout?
KJ
Comment #11
webchickI think it's in the theme function so the designer has control over deciding not to include that file.
Comment #12
kasperjames commentedre:staying on top of design and site knowledge.
i noticed that partaking in the current topics and "situations" isnt particulary easy,
and that perhaps adding in comments or asking questions here may get on peoples nevers, let alone possibly conflict with information and communication re: the task at hand, but i wanted to know.
is a multi user com messenger around here to help keep people like me from "cutting/butting in" so abruptly...
and while i wish to contribute to the over all efforts, sitting back and obsorbing information and the user/contributer roles is just as nice.
but im thinking it would be just as nice to obtain newbie feedback, and exchange questions and comments on the issues and circumstances at hand.
any ideas?
ty,
KJ
ps. would something short of a "page break" or "boxed-table" in "eughhh.png work?" help relieve the "spaceing(?)/layout issue". because obviously the br-type-tag is nice, but it seems this is a, what? "situation"?
Comment #13
Anonymous (not verified) commentedI disagree with above comments re: applying style to the module's css file. A normal user shouldn't have to override the module css just so that something looks decent. Styling for this should be in the default themes. Just adding margin doesn't create a clear enough difference.
Comment #14
webchick@kasperjames:
Hm. Sorry. I didn't really understand your comment. There is no community manager here; we're all just individuals working away on patches. :) If you have something to add on an issue, feel free to do so!
@both:
Yes, there's lots of ways we could make this prettier and more appealing. This is why I think it's best to -- for now -- commit the simple patch that just makes it tolerable, and then in a subsequent issue, play around with CSS to make it actually look /good/. ;) Since "good" is so subjective, there can be a lot of back/forth about the best ways to go about it, but the 'critical' piece we need at the moment is just for the text to not be smooshed up like that.
Comment #15
kasperjames commentedsomeone should make a drupal messenger. with features that pertain to this field of work.
thats all im saying.
Comment #16
catchAgree with webchick, one patch at a time, this one looks fine.
Comment #17
Anonymous (not verified) commented@webchick +1 I'll open a separate issue to prettify core themes for this item at some point this weekend, or you can go ahead ;)
Comment #18
gábor hojtsyOK, committed #8.
Comment #19
Anonymous (not verified) commentedAutomatically closed -- issue fixed for two weeks with no activity.