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

D8CORE-4778: adding underline hovers to splash links #244

Open
wants to merge 6 commits into
base: 9.x
Choose a base branch
from

Conversation

jenbreese
Copy link
Contributor

@jenbreese jenbreese commented Jun 2, 2023

READY FOR REVIEW

Summary

  • Adding an underline to the Splash text

Review By (Date)

  • Next week

Criticality

  • Normal

Urgency

  • Normal

Review Tasks

Setup tasks and/or behavior to test

  1. Check out this branch
  2. Rebuild Cache and import config drush cr ; drush ci
  3. Navigate to a page and create splash text with a link and one with a mailto link
  4. Verify there is an underline on hover.

Site Configuration Sync

  • Is there a config:export in this PR that changes the config sync directory?

Front End Validation

Backend / Functional Validation

Code

  • Are the naming conventions following our standards?
  • Does the code have sufficient inline comments?
  • Is there anything in this code that would be hidden or hard to discover through the UI?
  • Are there any code smells?
  • Are tests provided? eg (unit, behat, or codeception)

Code security

General

  • Is there anything included in this PR that is not related to the problem it is trying to solve?
  • Is the approach to the problem appropriate?

Affected Projects or Products

  • Does this PR impact any particular projects, products, or modules?

Associated Issues and/or People

- D8CORE-4778

Resources

@jenbreese jenbreese requested a review from jdwjdwjdw June 9, 2023 16:03
Copy link
Contributor

@jdwjdwjdw jdwjdwjdw left a comment

Choose a reason for hiding this comment

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

This looks good A11y-wise, but do you think we should we update this to match the other mailto link underlines? Those have the underline appearing as the default state, not just on hover.

@jenbreese
Copy link
Contributor Author

@jdwjdwjdw I'm not sure. @cjwest Does the underline on the splash text need to be on hover or a constant? I did it as an on hover. But Jacob points out that the other mailto are always underlined and the color changes on hover. thanks!

@cjwest
Copy link
Member

cjwest commented Jun 9, 2023

Good question, @jenbreese !
My understanding of this has changed over time. I agree with @jdwjdwjdw's point. Here's what the standard says:

To meet WCAG 1.4.1: Use of Color from Technique G183

underlines are recommended for links in blocks of text.

Copy link
Contributor

@pookmish pookmish left a comment

Choose a reason for hiding this comment

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

Actually, instead of ADDing more css, let's remove the css that this PR is overridding.

@jenbreese
Copy link
Contributor Author

jenbreese commented Jun 28, 2023

@pookmish I looked and the text-decoration:none comes from Decanter in the @types mixin: core/src/scss/utilities/mixins/typography/_types.scss

That mixin is used on these pages. The headings and the card would like have pretty big impact. It is safer to override rather than remove it in Decanter. I'm concerned it will have trickle effects that I would have to have other teammates agree on. What do you think?

Where the mixin is used:
decanter/core/src/scss/components/date-stacked/_date-stacked.scss
decanter/core/src/scss/elements/typography/_typography.scss - all headings
decanter/core/src/scss/utilities/mixins/card/_card.scss - on the super headline span
decanter/core/src/scss/utilities/mixins/typography/_font-splash.scss

@pookmish
Copy link
Contributor

pookmish commented Jun 28, 2023

Then this issue should really be resolved in decanter. Basically we should never do a text-decoration: none without re-setting the decoration on hover. There always needs to be a visual, non-color indicator for hover/focus. all that would really need to be done is add this code block to the @mixin types

 &:hover,
 &:focus {
   text-decoration: underline;
 }

These really should be any link in the affected elements you mentioned. The date stacked doesn't contain any links. The super headline doesn't contain any links. the heading should or already do have an underline if they are a link.

@pookmish
Copy link
Contributor

What it should look like
image

@jenbreese
Copy link
Contributor Author

Ok. That all makes sense. I will create that PR change for Decanter.

For the splash text, having the underline on hover will solve the original problem in the Jira, correct @cjwest?

@cjwest
Copy link
Member

cjwest commented Jun 28, 2023

@jenbreese
We want to treat mailto links just like other links: They are always underlined and the color changes on hover.

@pookmish pookmish removed their assignment Aug 10, 2023
@pookmish
Copy link
Contributor

This still needs work.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants