Skip to content

$link-decoration: underline affects some components#30262

Merged
MartijnCuppens merged 16 commits into
twbs:masterfrom
arthurshlain:patch-1
Mar 5, 2020
Merged

$link-decoration: underline affects some components#30262
MartijnCuppens merged 16 commits into
twbs:masterfrom
arthurshlain:patch-1

Conversation

@arthurshlain

@arthurshlain arthurshlain commented Feb 20, 2020

Copy link
Copy Markdown

Removed redundant text-decoration when element is link with .btn class (a.btn) and $link-decoration variable is set to 'underline'.

.btn-link also works with this fix, because .btn-link declaration located below.

Upd.

Fixed text-decoration for:

  • .btn (except .btn-link)
  • .dropdown-item
  • .list-group-item
  • .page-link (looks like button)
  • .nav-link

Removed .btn redundant text-decoration when dom element is link (a.btn) and $link-decoration variable is set to 'underline'.
ysds
ysds previously approved these changes Feb 20, 2020

@ysds ysds left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nice catch!

@ysds

ysds commented Feb 20, 2020

Copy link
Copy Markdown
Contributor

I'm wondering if this should be applied to other components.

@arthurshlain

Copy link
Copy Markdown
Author

I know same behavior exist with pagination elements.
Also 'underline' applied to .navbar-brand, but it's maybe ok...

Removed redundant text-decoration when element is link with .page-link class (a.page-link) and $link-decoration variable is set to 'underline'.
@arthurshlain arthurshlain changed the title Update _buttons.scss $link-decoration: underline affects some components Feb 20, 2020
.page-link property ordering
.page-link property ordering
.page-link property ordering
.page-link property ordering
@MartijnCuppens

Copy link
Copy Markdown
Member

I would prefer an if test to prevent generation of the property if not needed. Something like:

text-decoration: if($link-decoration == none, null, none);

(code not tested)

@arthurshlain

Copy link
Copy Markdown
Author

(code not tested)

It's works good

check $link-decoration variable for .btn "text-decoration: none"
check $link-decoration variable for .page-link "text-decoration: none"
.nav-link text-decoration: none;
.nav-link attr ordering
text-decoration: none; for .dropdown-item
text-decoration: none; for .list-group-item
@arthurshlain

arthurshlain commented Feb 20, 2020

Copy link
Copy Markdown
Author

.nav-link looks normal by default, except .active state.

image

Remove underline for any .nav-link or only for .active state?

@MartijnCuppens

Copy link
Copy Markdown
Member

Remove underline for any .nav-link or only for .active state?

I would just make sure the nav links look the same like in the current docs:

image

@arthurshlain

Copy link
Copy Markdown
Author

Something wrong with this fix now?

@ysds ysds left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM

@MartijnCuppens MartijnCuppens merged commit d9215eb into twbs:master Mar 5, 2020
@patrickhlauke

Copy link
Copy Markdown
Member

this closed #15304 but note that one of the scenarios is still there...alerts with links and strong text (mocked up here by taking one of the alerts in the docs and adding an <a href="#" class="alert-link">...</a> in there)

screenshot of alert with both bold strong text and a bold link

@MartijnCuppens

Copy link
Copy Markdown
Member

Didn't notice this was added, I've reopened #15304.

XhmikosR pushed a commit that referenced this pull request Mar 5, 2020
Co-authored-by: Shohei Yoshida <fellows3@gmail.com>
Co-authored-by: Martijn Cuppens <martijn.cuppens@gmail.com>
@arthurshlain arthurshlain deleted the patch-1 branch March 5, 2020 17:05
XhmikosR pushed a commit that referenced this pull request Mar 9, 2020
Co-authored-by: Shohei Yoshida <fellows3@gmail.com>
Co-authored-by: Martijn Cuppens <martijn.cuppens@gmail.com>
@mdo mdo mentioned this pull request Mar 12, 2020
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
Co-authored-by: Shohei Yoshida <fellows3@gmail.com>
Co-authored-by: Martijn Cuppens <martijn.cuppens@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants