The theme_links() function adds a new line after list elements (by adding '\n'). However, this leads to whitespace
in many browsers between the list elements when aligned horisontally. Even though it does lead to code which
looks a little less nice, would it be an idea to remove the new line (\'n')?

I can't really see any way to deal with this through CSS, as the browsers seems to convert the new line to space
when list elements are aligned horisontally.

CommentFileSizeAuthor
#10 theme_links-remove-newlines_3.patch1.61 KBJeff Burnz
#4 theme_links-remove-newlines_2.patch1.61 KBJeff Burnz
#1 theme_links-remove-newlines.patch1.61 KBJeff Burnz

Comments

Jeff Burnz’s picture

Jeff Burnz commented
Component: theme system » markup
Status: Active » Needs review
StatusFileSize
new theme_links-remove-newlines.patch1.61 KB

I think this is a good move, that whitespace has plauged me for years, driving me to override theme_links to get rid of it. I think nicely formatted output takes a distant second place to theme-ability in this instance.

Bartik requires a small change also.

Changing component to get the markup peeps eyes on this.

Jeff Burnz’s picture

Jeff Burnz commented
Title: theme_links adds a new line ('\n') after <li> - leads to space between horisontally aligned list elements » theme_links adds a new line ('\n') after <li> - leads to space between horizontally aligned list elements

fix typo in title.

Status: Needs review » Needs work

The last submitted patch, theme_links-remove-newlines.patch, failed testing.

Jeff Burnz’s picture

Jeff Burnz commented
Status: Needs work » Needs review
StatusFileSize
new theme_links-remove-newlines_2.patch1.61 KB

My bad, ran it against the wrong version of theme.inc, this should be OK.

Status: Needs review » Needs work

The last submitted patch, theme_links-remove-newlines_2.patch, failed testing.

Jeff Burnz’s picture

Jeff Burnz commented

I don't understand the fail, revisions are correct at this time so not sure what the problem is.

jacine’s picture

jacine
she/her
Location New York
commented

Hmm, what is the Bartik code doing in this patch? Doesn't seem like it has anything to do with it.

One of the hunks failed in style-rtl.css:

[13:21:07] Command [patch -p0 -i /var/lib/drupaltestbot/sites/default/files/review/theme_links-remove-newlines_2.patch 2>&1] failed with status [1] and output:
patching file themes/bartik/css/style-rtl.css
Hunk #1 FAILED at 80.
1 out of 1 hunk FAILED -- saving rejects to file themes/bartik/css/style-rtl.css.rej
patching file themes/bartik/css/style.css
Hunk #1 FAILED at 426.
1 out of 1 hunk FAILED -- saving rejects to file themes/bartik/css/style.css.rej
patching file includes/theme.inc
Hunk #1 succeeded at 1454 (offset -10 lines)..
jacine’s picture

jacine
she/her
Location New York
commented
Version: 7.0-alpha6 » 7.x-dev
Jeff Burnz’s picture

Jeff Burnz commented

Because this change breaks Bartik - we need to include the fix (the tabs get butted up against each other, they are relying on this white space for "padding").

Jeff Burnz’s picture

Jeff Burnz commented
Status: Needs work » Needs review
StatusFileSize
new theme_links-remove-newlines_3.patch1.61 KB

If this fails I must be doing something really wrong here, rolling patches all day and all passes so a bit weird this one keeps failing...

bleen’s picture

bleen commented

Jeff Burnz: this usually means that testbot has been drinking again

sbrattla’s picture

sbrattla commented

I see that the very same issue can be found in theme_menu_link(), theme_menu_local_task() and theme_menu_local_action (in menu.inc) too. A newline is added to after each list element. Is that something we should address as well?

jacine’s picture

jacine
she/her
Location New York
commented

I dunno... I'm kinda torn on this.

damien tournoud’s picture

damien tournoud commented

Is this browser behavior by spec? Why is this space significant here?

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

This fix, along with some others, is already contained in #98696: Various bugs in theme_links()

I agree that we need to fix this bug, but I'd highly prefer to do #98696

Jeff Burnz’s picture

Jeff Burnz commented

Its significant because there is an actual white-space between the list items - even if you set padding 0 margin 0 this "gap" remains, this causes all sorts of hilarity depending on how you theme your links, it would be much easier if this white-space was not there.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented

@Jeff: I totally agree with you. I had to override theme_links() as well as theme_item_list() for every site that I've built since Drupal 5.

However, as mentioned in #15 already, it's rather the entire theme functions that badly need a simplification. And that's what #98696: Various bugs in theme_links() and #256827: Various bugs in theme_item_list() are doing -- which both include this very bug fix.

sun’s picture

sun
Primary language German
Location Karlsruhe
commented
Version: 7.x-dev » 8.x-dev
Status: Needs review » Closed (duplicate)

Since #98696: Various bugs in theme_links() has been bumped to D8 - although it fixes an entire list of bugs (including this one) - this one is equally D8 material. Note that the Edge project will provide the cleaned up and fixed theme functions.

And since we will want to do #98696 for D8, I'm marking this issue as duplicate.