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

Skip to main content link #1857

Merged
merged 6 commits into from Jan 24, 2024
Merged

Skip to main content link #1857

merged 6 commits into from Jan 24, 2024

Conversation

machikoyasuda
Copy link
Member

@machikoyasuda machikoyasuda commented Jan 19, 2024

closes #1852

What this PR does

  • Restyles Skip to main content link focus ring
  • Refactors focus ring CSS to use variables, so both rings can use same variable

A11y testing this branch versus test:
image

image

Copy link

Coverage report

This PR does not seem to contain any modification to coverable code.

@github-actions github-actions bot added deployment-dev [auto] Changes that will trigger a deploy if merged to dev i18n Copy: Language files or Django i18n framework front-end HTML/CSS/JavaScript and Django templates and removed deployment-dev [auto] Changes that will trigger a deploy if merged to dev labels Jan 19, 2024
@machikoyasuda machikoyasuda changed the title wip Skip Nav Jan 19, 2024
@machikoyasuda machikoyasuda changed the title Skip Nav Skip to main content link Jan 19, 2024
@machikoyasuda machikoyasuda self-assigned this Jan 19, 2024
@machikoyasuda machikoyasuda marked this pull request as ready for review January 19, 2024 01:17
@machikoyasuda machikoyasuda requested a review from a team as a code owner January 19, 2024 01:17
@@ -44,7 +44,7 @@
{% endif %}
<header role="banner" id="header">
<a id="skip-to-content" href="#main-content" class="d-block w-100">
<div class="container">{% translate "Skip to Main Content" %}</div>
<div class="container"><span>{% translate "Skip to main content" %}</span></div>
Copy link
Member Author

Choose a reason for hiding this comment

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

Added this span here for semantic-ness.

Copy link
Member

Choose a reason for hiding this comment

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

Can you elaborate a bit? Both <div> and <span> carry no semantic meaning on their own, the difference being denoting either block-level or inline content. Text content is allowed inside both of them...

If there is an accessibility issue here with semantics, wouldn't a <p> be more appropriate?

Copy link
Member Author

Choose a reason for hiding this comment

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

Semantic was the wrong word here. Though I'm not sure what the right would be. Having a border (for the focus) around a p or any block element (like p or div) would make it look like this, with the yellow border around the entire div:
image

But a span is an inline-display element, so the same CSS renders the focus like this instead:
image

So it's not semantic-ness but more like....... the native HTML element's inherit default block-or-inline-layout-ness.

Comment on lines 298 to 299
border: 2px solid var(--focus-color);
border-radius: calc(4rem / 16);
Copy link
Member Author

Choose a reason for hiding this comment

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

Need to clear up one thing: https://www.figma.com/file/SeSd3LaLd6WkbEYhmtKpO3?node-id=438:1160&mode=dev#673932531

@indexing @segacy1 The standard yellow focus ring style has a border-radius of 2px and a border of 3px. This one is 1px off for both. Is that okay, or should I use the standard yellow focus ring style?

Copy link

Choose a reason for hiding this comment

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

Let's stick with what's standard! I updated Figma to match. ty!

Copy link
Member Author

Choose a reason for hiding this comment

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

image
Now matches the other link focus ring style.

@machikoyasuda machikoyasuda marked this pull request as draft January 19, 2024 01:27
@machikoyasuda machikoyasuda marked this pull request as ready for review January 22, 2024 21:23
Copy link
Member

@thekaveman thekaveman left a comment

Choose a reason for hiding this comment

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

Looks great on my machine!

image

@machikoyasuda machikoyasuda merged commit 5f41656 into dev Jan 24, 2024
8 checks passed
@machikoyasuda machikoyasuda deleted the fix/1852-skipnav branch January 24, 2024 02:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
front-end HTML/CSS/JavaScript and Django templates i18n Copy: Language files or Django i18n framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix color contrast in Skip Nav
3 participants