Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change deprecated html tags to text decoration classes #29604

Merged
merged 4 commits into from Nov 27, 2019

Conversation

mattvgn
Copy link
Contributor

@mattvgn mattvgn commented Oct 28, 2019

@mattvgn mattvgn requested a review from a team as a code owner October 28, 2019 21:48
MartijnCuppens
MartijnCuppens previously approved these changes Oct 29, 2019
Copy link
Member

@MartijnCuppens MartijnCuppens left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@florianlacreuse
Copy link
Contributor

Thanks @mattvgn!

It would be perfect if this could be backported to v4, if possible.

@mdo
Copy link
Member

mdo commented Oct 30, 2019

These elements were restored in HTML5.

Along with other pure styling elements, the original HTML Underline () element was deprecated in HTML 4; however, was restored in HTML 5 with a new, semantic, meaning: to mark text as having some form of non-textual annotation applied.

https://developer.mozilla.org/en-US/docs/Web/HTML/Element/u

@XhmikosR
Copy link
Member

Yup, they are not deprecated any more; their meaning is different.

@mattvgn
Copy link
Contributor Author

mattvgn commented Oct 30, 2019

This is not deprecated but people might use it wrongly :

You should not use to simply underline text for presentation purposes, or to denote titles of books.

About the <s> nothing in the documentation is telling to not use it : https://developer.mozilla.org/en-US/docs/Web/HTML/Element/s

What about removing the <u> in the "Inline text elements" section on Bootstrap and let the <s>, but still add this two classes : .text-decoration-underline .text-decoration-line-through ?

@florianlacreuse
Copy link
Contributor

I agree with @mattvgn. As I said in my issue #29584, we should not use <u> as we used to do in the past (highlight / focus a piece of text).

@mattvgn
Copy link
Contributor Author

mattvgn commented Nov 5, 2019

This is not deprecated but people might use it wrongly :

You should not use to simply underline text for presentation purposes, or to denote titles of books.

About the <s> nothing in the documentation is telling to not use it : https://developer.mozilla.org/en-US/docs/Web/HTML/Element/s

What about removing the <u> in the "Inline text elements" section on Bootstrap and let the <s>, but still add this two classes : .text-decoration-underline .text-decoration-line-through ?

@mdo @MartijnCuppens @XhmikosR What do you guys think ?

@MartijnCuppens
Copy link
Member

We might need to add some documentation about <u>, <s> (& <b> while we're at it) to clarify their meaning.

I think we still need the newly added utilities here, to separate semantic elements from styling classes.

v5 automation moved this from Inbox to Approved Nov 26, 2019
@mattvgn
Copy link
Contributor Author

mattvgn commented Nov 26, 2019

Capture d’écran 2019-11-26 à 21 57 39

Here is the screenshot of the documentation based on your feedback

@florianlacreuse
Copy link
Contributor

@mattvgn May be you already know that but the documentation is built for each PR, see deploy notifiy check / job in the below approval section. Current direct link for instance: https://deploy-preview-29604--twbs-bootstrap.netlify.com/docs/4.3/content/typography/#inline-text-elements

@XhmikosR
Copy link
Member

Do we want to backport this in v4-dev @mdo @MartijnCuppens?

@XhmikosR XhmikosR merged commit 4de4874 into twbs:master Nov 27, 2019
v5 automation moved this from Approved to Shipped Nov 27, 2019
@MartijnCuppens
Copy link
Member

Do we want to backport this in v4-dev @mdo @MartijnCuppens?

This can be seen as a new feature, not sure if we want to release v4.5?

@XhmikosR
Copy link
Member

NP, let's leave it in v5 for now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
v5
  
Shipped
Development

Successfully merging this pull request may close these issues.

None yet

5 participants