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

Create a no-link-underline CSS helper #324

Open
mansona opened this issue May 1, 2020 · 5 comments
Open

Create a no-link-underline CSS helper #324

mansona opened this issue May 1, 2020 · 5 comments
Labels
good first issue We really need your help! good first issue 🥇 New here? Try one of these!

Comments

@mansona
Copy link
Member

mansona commented May 1, 2020

There are enough cases where we would like to be able to turn off the default link underline behaviour in designs that it makes sense for us to add a helper that sets background: none to overcome this for us.

You can see a good example of the need for this helper in the discussion on ember-learn/empress-blog-ember-template#26

If you implement this helper you will need to make sure that it is documented 👍

@mansona mansona added good first issue 🥇 New here? Try one of these! good first issue We really need your help! labels May 1, 2020
@alexeykostevich
Copy link
Contributor

alexeykostevich commented May 14, 2020

Currently, the Ember Styleguide provides the .bg-none CSS helper to turn off link-underline styles:
https://ember-styleguide.netlify.app/css/helpers (see "Turn off link-underline styles").

However, the helper works only for links scoped to the main tag since they use background for making a custom "fat" underline.

background: no-repeat left bottom

All other links use text-decoration: underline; and therefore will not be affected by .bg-none:

text-decoration: underline;

So, there are 3 solutions for this issue:

  1. Use only background-based underline for all the links and avoid using text-decoration: underline;. This will require to update some apps to take into account the change.
  2. Add a new helper .no-link-underline that will handle all possible cases.
.no-link-underline
.no-link-underline:link,
.no-link-underline:visited {
  background: none;
  text-decoration: none;
}

@alexeykostevich
Copy link
Contributor

Per the discussion with @mansona , option 1 looks better. However, we need to be cautious not to break the existing apps.

@mansona
Copy link
Member Author

mansona commented May 15, 2020

So there is another way of thinking about this whole situation 🤔

I happen to agree with the default styles being applied to main. Sure we could extend it to be in all cases but that then goes against the point of why it was only applied to main, we want to be able to have default link styles for the main content of the site and have them not apply to the nav or the footer.

For this reason I don't think we should be doing option 1.

I think the best way forward would be to go for option 2. but for us to use a "deprecation" methodology, so we should be able to use either the new class .no-link-underline or the old one .bg-none to have the same effect. We can worry about how we actually deprecate the use of .bg-none later.

Does that make sense?

@alexeykostevich
Copy link
Contributor

alexeykostevich commented May 17, 2020

Thank you for looking at this, @mansona !

Looking at this again, I've realized that all of these solutions have their own pros and cons. Using background for custom "fat" underline makes this a bit tricky 🙂 Please, let me explain this in more detail.

Overall, the issue happens since we have two ways of making text underlined (using background and text-decoration) but the helper for turning off underlines handles only one of them.

Here is the simplified version of our styles:

/* Underline links by default using `text-decoration`. */
a {
  text-decoration: underline;
}

/* Underline links inside <main> using `background`. */
main a {
  text-decoration: none;
  background: no-repeat left bottom
    linear-gradient(var(--color-brand-40), var(--color-brand-40));
  background-size: 100% 0.1875rem;
}

/* The helper handles only `background`-based underlines. */
.bg-none,
.bg-none a {
   background: none;
}

Option 1: Use only background-based underline

We can consistently use background-based approach for handling underlines. This does not mean that all links should have the same underline — they just need to share the same approach of doing this.

Technically speaking, it could look like this:

/* Use `background` for handling all underlines. */
a {
  background: no-repeat left bottom
    linear-gradient(var(--color-brand-hc-dark), var(--color-brand-hc-dark));
  background-size: 100% 1px;
  color: var(--color-brand-hc-dark);
  text-decoration: none;
}

main a {
  color: var(--color-link);
  text-decoration: none;
  background: no-repeat left bottom
    linear-gradient(var(--color-brand-40), var(--color-brand-40));
  background-size: 100% 0.1875rem;
}

/* This helper can be used to turn off underlines. */
.bg-none,
.bg-none a {
   background: none;
}

Pros

  • A consistent approach for handling underlines.
  • We will continue using the existing .bg-none helper for turning off all underlines.

Cons

  • Using .bg-none for turning off underlines is not obvious.
  • Using background-based underlines do not work well with block elements (especially, with padding, custom background, etc.). Likely, this is why we do not use this approach outside of main: making a as a block element for links is common in headers and navbars.
  • The change will require additional changes in places that rely on the existing link styles.

Frankly, I totally agree with you and don't recommend to go with this approach because of these cons.

Option 2: Keep the existing styles but add a new helper

We can add a new helper .no-link-underline that will handle all possible all ways of underlining links. We don't need to change or deprecate .bg-none since it still has valid use cases besides underlines.

.bg-none {
  background: none;
}

/* This helper can be used to turn off underlines regardless of what approach is used. */
.no-link-underline,
.no-link-underline:link,
.no-link-underline:visited {
  background: none;
  text-decoration: none;
}

Pros

  • A predictable and obvious approach for turning underlines.

Cons

  • .no-link-underline will have a not obvious side-effect: removing background from the element, which can be unexpected in some scenarios.

This solution looks pretty good but let's talk about another one to get the full picture.

Option 3: Define only default styles for links and override where needed

We can define the default links styles and explicitly override them in components where they should look differently.

a {
  text-decoration: none;
  background: no-repeat left bottom
    linear-gradient(var(--color-brand-40), var(--color-brand-40));
  background-size: 100% 0.1875rem;
}

/* This helper can be used to turn off underlines. */
.bg-none,
.bg-none a {
   background: none;
}

Pros

  • A consistent approach for handling underlines.
  • Reduces selector specificity and overall complexity.

Cons

  • The change will require additional changes in places that rely on the existing link styles.

Interestingly, if we have a look at https://emberjs.com/, we will notice that links outside of main do not use any underlines at all 🙂 For example, the EsHeader and EsFooter already explicitly override link styles:

So, I would recommend to consider this approach as a long-term solution.

@pichfl , please, could you have a look a this and let us know you opinion? You was an author of some related changes (85e0489) and don't want to miss something important.

Thank you!

@neojp
Copy link
Contributor

neojp commented Nov 1, 2020

I just happened to run into this in another commit in which .bg-none wouldn't hide the link's background because main a has a higher priority than .bg-none. I ended up having to write custom css to override these styles.

Note: .toc-anchor is a link inside main.

https://github.com/ember-learn/deprecation-app/blob/542f6b9761148a371727d3df9a311e2cf4fdf076/app/styles/app.scss#L119-L123

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue We really need your help! good first issue 🥇 New here? Try one of these!
Projects
None yet
Development

No branches or pull requests

3 participants