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.

Sign up for GitHub

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

Jump to bottom

$link-decoration: underline affects some components #30262

Merged
merged 16 commits into from
Mar 5, 2020
Merged

$link-decoration: underline affects some components #30262

merged 16 commits into from
Mar 5, 2020

Conversation

arthurshlain
Copy link

@arthurshlain arthurshlain commented Feb 20, 2020 •
edited
Loading

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

@arthurshlain
Update _buttons.scss
aca3080 
Removed .btn redundant text-decoration when dom element is link (a.btn) and $link-decoration variable is set to 'underline'.
@arthurshlain arthurshlain requested a review from a team as a code owner February 20, 2020 13:21
ysds
ysds previously approved these changes Feb 20, 2020
View reviewed changes
Copy link
Contributor

@ysds ysds left a comment

Choose a reason for hiding this comment

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

Nice catch!

@ysds
Copy link
Contributor

ysds commented Feb 20, 2020

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

@arthurshlain
Copy link
Author

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

@arthurshlain
Update _pagination.scss
37ad111 
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
@arthurshlain
Update _pagination.scss
c4de21e 
.page-link property ordering
@arthurshlain
Update _pagination.scss
9e74c10 
.page-link property ordering
@arthurshlain
Update _pagination.scss
f99d5d0 
.page-link property ordering
@arthurshlain
Update _pagination.scss
955caa9 
.page-link property ordering
@MartijnCuppens
Copy link
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
Author

(code not tested)

It's works good

@arthurshlain
Update _buttons.scss
2204f97 
check $link-decoration variable for .btn "text-decoration: none"
@arthurshlain
Update _pagination.scss
6c917e6 
check $link-decoration variable for .page-link "text-decoration: none"
@arthurshlain
Update _nav.scss
2af07d3 
.nav-link text-decoration: none;
@arthurshlain
Update _nav.scss
b0f0c36 
.nav-link attr ordering
@arthurshlain
Update _dropdown.scss
d301554 
text-decoration: none; for .dropdown-item
@arthurshlain
Update _list-group.scss
523cdc4 
text-decoration: none; for .list-group-item
@arthurshlain
Copy link
Author

arthurshlain commented Feb 20, 2020 •
edited
Loading

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

image

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

@MartijnCuppens
Copy link
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

@ysds ysds self-requested a review February 21, 2020 10:30
@arthurshlain
Copy link
Author

Something wrong with this fix now?

ysds
ysds approved these changes Feb 24, 2020
View reviewed changes
Copy link
Contributor

@ysds ysds left a comment

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

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

XhmikosR pushed a commit that referenced this pull request Mar 5, 2020
@MartijnCuppens @ysds @XhmikosR
$link-decoration: underline affects some components (#30262)
be947ff 
Co-authored-by: Shohei Yoshida <[email protected]>
Co-authored-by: Martijn Cuppens <[email protected]>
@arthurshlain arthurshlain deleted the patch-1 branch March 5, 2020 17:05
XhmikosR pushed a commit that referenced this pull request Mar 9, 2020
@MartijnCuppens @ysds @XhmikosR
$link-decoration: underline affects some components (#30262)
efb3fc4 
Co-authored-by: Shohei Yoshida <[email protected]>
Co-authored-by: Martijn Cuppens <[email protected]>
@mdo mdo mentioned this pull request Mar 12, 2020
Merged
@ffoodd ffoodd mentioned this pull request Jun 10, 2020
Merged
olsza pushed a commit to olsza/bootstrap that referenced this pull request Oct 3, 2020
@MartijnCuppens @ysds @olsza
$link-decoration: underline affects some components (twbs#30262)
69547e6 
Co-authored-by: Shohei Yoshida <[email protected]>
Co-authored-by: Martijn Cuppens <[email protected]>
@nelsonni nelsonni mentioned this pull request Jan 5, 2021
Merged
@jcandan jcandan mentioned this pull request Sep 19, 2023
Open
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ysds ysds ysds approved these changes

@MartijnCuppens MartijnCuppens MartijnCuppens approved these changes

Assignees
No one assigned
Labels
backport-to-v4 css v5
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@arthurshlain @ysds @MartijnCuppens @patrickhlauke