Shouldn't post-controls buttons use <div> instead of <i>?


#1

Most1 of the buttons under the post-controls->actions section are using the i tag to place their icon, and this style is even used in one tutorial here:

Given that the bookmark button is using the div tag already, wouldn’t it be appropriate to convert all such tags to use div instead of i?


1: Exceptions are the bookmarks icon and buttons with text instead of an icon.

(Mittineague) #2

AFAIK the many i tags are everywhere there is a FontAwesome glyph

Changing to div tags seems like it would easy, changing the CSS a nightmare


(Sam Saffron) #3

Technically it may be a bit more semantic to just have an empty button or a button with text and then render text offscreen.

But… it would be a major amount of internal restructuring for very small gain (we already have aria-label on the button)


(Mittineague) #4

As a veteran of the “div-itis” years I am a bit surprised that the i tag bothers me not in the slightest.
I guess the relaxed standards of HTML5 have conditioned me.

Anyway, I tried finding an i tag that had no FontAwesome glyph, but failed.
(I apologize if I missed it)

@Dreikin got an example?


#5

What would CSS need to be changed for? Testing in Chrome’s developer tools, i -> div works fine:


Hm, okay, how about just putting the class="fa fa-something" on the button itself?:


Nope. Going by what @Mittineague said, I guess they’re the only ones done that way. Which would make the FontAwesome ones inconsistent with all the others instead of just a few.

Coming back to this for a different reason:
At least some of the buttons seem to be defined/setup here:
https://github.com/discourse/discourse/blob/master/app/assets/javascripts/discourse/components/post-menu.js.es6
Line 45 (in Button.prototype.render = [...]) calls `iconHTML():

if (this.icon) { buffer.push(iconHTML(this.icon)); }

Which is defined in:


And you can see in the onebox that that is where the i tag is defined to be used instead of div (or something else).

So it seems to me the easy fix would be to change i to div in the second source file. To go a bit further, one could modify iconHTML to return class="fa fa-something" in the case of no params passed, then change the Button.prototype.render function to add the result of that call to the button, rather than to an subsidiary div (or, in the current case, i). I’m not sure how safe that second modification is because of what it does if you pass a second argument, however.


(Mittineague) #6

Unfortunately not so easy.
The i tags have ::before
The glyph is contained by the div


#7

Could you rephrase please? I’m not following the importance of that (this is not one of my areas of expertise). Is the ::before used somewhere that would affect buttons styled thusly? And does “The glyph is contained by the div” matter with the second example where the button itself is given the class?


(Sam Saffron) #8

The bookmark thing is historic btw… we used to display something different depneding on if you read or did not read the post, it used to change color.

We probably should convert it to an i just to be consistent.

Though I am not sure what real world problem this solves.


(Mittineague) #9

I’ve been trying to think of possible reasons

It does seem wrong to use an italics tag with nothing in it and only used to ::before an icon.

But as far as semantics, the strong tag has the meaning. AFAIK italics is a sometimes-OK-to-use tag used for styling. eg. publication reference and scientific naming.

I don’t see any real harm from having empty i tags.
And I’ve seen plenty of sites that had empty tags.
Doesn’t make it “100% right” but I guess it doesn’t mean it’s totally wrong either.

Just something to “hook” to. And if changing would take a lot of effort I can think of higher priority tasks.


#10

Well sure…but then you have to change other buttons to be consistent too (e.g., the editing buttons apparently are styled by the button’s class.

Looking into it more, it appears that the expand/collapse button is another i tag - inside a div, not a button. Hm.



Changing that to a div works fine, though.

However, the Reply button looks wrong either with the class inside the button (top is edited, bottom is normal):

or with the i changed to div:

Though the latter can be fixed by adding a space before “Reply”:

Reply as linked Topic is the most broken when changed I’ve found so far. In addition to losing spacing, the + gets bigger and doesn’t have that nice blue background highlight effect.

A personal case of very, very misguided SIWOTI? :wink:


(Kane York) #11

The <i> tag is used because:

  • it’s an inline tag, not a block tag - you should be proposing it to be replaced with span, not div

  • it’s what Font Awesome recommends: fa-check: Font Awesome Icons

     <i class="fa fa-check"></i>

#12

Ahh…I forgot that distinction (and didn’t realize it was relevant for buttons). I’ll have to mess around with that when I’m at my computer again.

Well sure, go do the reasonable thing and follow the documentation. I wonder why they chose to do it that way.

Thanks for the rationale :slight_smile:


(Jakob Borg) #13

I suspect this is just a repurposing of an otherwise seldom used tag that looks like it could be a mnemonic for icon.