Skip to content

Misused i element replaced with neutral span#1668

Merged
tadatuta merged 1 commit intobem:v2from
pepelsbey:v2
Sep 17, 2015
Merged

Misused i element replaced with neutral span#1668
tadatuta merged 1 commit intobem:v2from
pepelsbey:v2

Conversation

@pepelsbey
Copy link
Contributor

@pepelsbey pepelsbey commented Sep 10, 2015

<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
Copy link
Member

tadatuta commented Sep 10, 2015

@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
Copy link
Contributor Author

pepelsbey commented Sep 10, 2015

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
Copy link
Member

tadatuta commented Sep 10, 2015

@pepelsbey Fix is on the way

@pepelsbey
Copy link
Contributor Author

pepelsbey commented Sep 10, 2015

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
Copy link
Member

tadatuta commented Sep 10, 2015

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
Copy link
Contributor Author

pepelsbey commented Sep 10, 2015

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

@tadatuta
Copy link
Member

tadatuta commented Sep 10, 2015

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

@tadatuta
Copy link
Member

tadatuta commented Sep 10, 2015

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
Copy link
Contributor Author

pepelsbey commented Sep 10, 2015

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

@tadatuta
Copy link
Member

tadatuta commented Sep 15, 2015

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

@tadatuta
Copy link
Member

tadatuta commented Sep 16, 2015

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

@pepelsbey
Copy link
Contributor Author

pepelsbey commented Sep 16, 2015

@tadatuta, I hope I did it right

@qfox
Copy link
Member

qfox commented Sep 16, 2015

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
Copy link
Contributor Author

pepelsbey commented Sep 16, 2015

@zxqfox, thanks, it did the trick.

@qfox
Copy link
Member

qfox commented Sep 16, 2015

👍 Hope tests will pass. ;-)

tadatuta added a commit that referenced this pull request Sep 17, 2015
@tadatuta
Merge pull request #1668 from pepelsbey/v2
aea8f49 
Misused i element replaced with neutral span
@tadatuta tadatuta merged commit aea8f49 into bem:v2 Sep 17, 2015
@tadatuta
Copy link
Member

tadatuta commented Sep 17, 2015

@pepelsbey thank you!

@pepelsbey
Copy link
Contributor Author

pepelsbey commented Sep 17, 2015

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

@aristov
Copy link
Contributor

aristov commented Sep 22, 2015

@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.

@aristov aristov added the a11y label Sep 22, 2015
@pepelsbey
Copy link
Contributor Author

pepelsbey commented Sep 22, 2015

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

a11y

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@pepelsbey @tadatuta @qfox @aristov