Misused i element replaced with neutral span

[pepelsbey]

<i> element has a clear semantic role accoring to HTML spec:

The i element represents a span of text in an alternate voice or mood, or otherwise offset from the normal prose in a manner indicating a different quality of text, such as a taxonomic designation, a technical term, an idiomatic phrase from another language, transliteration, a thought, or a ship name in Western texts.

Using it for icons or any other decoration should be considered as bad practice. Bootstrap framework for example clearly made its choice to remove all non-semantic elements from decoration and starting from v2.1.0 <i class="icon"></i> has been replaced with <span> and starting from v3.2.0 all <b class="caret"></b> nonsense is gone as well.

Please show that you care about what you recommend developers to use.

[tadatuta]

@pepelsbey will you please also update tests (see https://travis-ci.org/bem/bem-components/jobs/79723658)?
Reference HTMLs are in *.blocks/*/*.tmpl-specs/*.html (e.g. https://github.com/bem/bem-components/blob/v2/common.blocks/icon/icon.tmpl-specs/10-default.html).

And it'd be great if you create an issue for this change following contribution guide.

[pepelsbey]

And it'd be great if you create an issue for this change following contribution guide

@tadatuta, I expected to find CONTRIBUTING.md file (see GitHub documentation), haven’t found it and decided to just PR first.

[tadatuta]

@pepelsbey Fix is on the way

[pepelsbey]

Speaking of <i aria-hidden="true"></i>, is it something you really need since it’s <span> now? I could ask a11y experts and file another PR for this.

[tadatuta]

Speaking of <i aria-hidden="true"></i>, is it something you really need since it’s <span> now? I could ask a11y experts and file another PR for this.

can't say for sure so will be grateful if you do

[pepelsbey]

So, should I cancel this PR and file a proper issue and another PR?

[tadatuta]

Just create an issue, copy description of the changes there and connect PR with it, that'll be enough.

[tadatuta]

Looks like there are stil 30 falling tests: https://travis-ci.org/bem/bem-components/jobs/79736711

That's because of BH templates (e.g. https://github.com/bem/bem-components/blob/v2/common.blocks/icon/icon.bh.js#L7).

[pepelsbey]

Thanks for the hint, I’ll make sure that all tests are fine.

[tadatuta]

@pepelsbey please try to rebase the PR, hope it'll pass afterwards

[tadatuta]

well, tests are green :)
can you please squash commits?

[pepelsbey]

@tadatuta, I hope I did it right

[qfox]

Ehm... "Merge branch" means you did it wrong ;-(

I'd do now:

git reset --hard upstream/v2
git cherry-pick --no-commit 994a501 6430ba6

and then commit again your changes and force push to the branch linked to this PR.

[pepelsbey]

@zxqfox, thanks, it did the trick.

[qfox]

👍 Hope tests will pass. ;-)

[tadatuta]

@pepelsbey thank you!

[pepelsbey]

@tadatuta, thank you for accepting it, and I’ve learned a lot during this PR :)

[aristov]

@pepelsbey 👍

I'm totally agree. If we still don't use native semantics, so we just should avoid deliberately wrong usage.

Speaking of <i aria-hidden="true"></i>, is it something you really need since it’s <span> now? I could ask a11y experts and file another PR for this.

Already removed and will be merged soon.

[pepelsbey]

@aristov, thank you for following up on that, I just got completely lost on vacation.