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

Skip to main content link #1857

Merged
merged 6 commits into from
Jan 24, 2024
Merged

Skip to main content link #1857

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7a8fcf8
fix(copy): lowercase main content
machikoyasuda Jan 19, 2024
cd64efc
feat(css): first pass @ new skip nav
machikoyasuda Jan 19, 2024
ed5d213
refactor(css): use variables
machikoyasuda Jan 19, 2024
807277d
fix(css): remove unnecessary style declarations
machikoyasuda Jan 19, 2024
d6b5410
fix: undo testing
machikoyasuda Jan 19, 2024
754b615
refactor(css): variable-ize focus style
machikoyasuda Jan 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion benefits/core/templates/core/base.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

</a>
{% if messages %}
{% for message in messages %}
Expand Down
4 changes: 2 additions & 2 deletions benefits/locale/en/LC_MESSAGES/django.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
msgid ""
msgstr ""
"Report-Msgid-Bugs-To: https://github.com/cal-itp/benefits/issues \n"
"POT-Creation-Date: 2023-12-14 21:35+0000\n"
"POT-Creation-Date: 2024-01-19 00:29+0000\n"
"Language: English\n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
Expand All @@ -28,7 +28,7 @@ msgstr ""
msgid "Cal-ITP Benefits"
msgstr ""

msgid "Skip to Main Content"
msgid "Skip to main content"
msgstr ""

msgctxt "image alt text"
Expand Down
4 changes: 2 additions & 2 deletions benefits/locale/es/LC_MESSAGES/django.po
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@
msgid ""
msgstr ""
"Report-Msgid-Bugs-To: https://github.com/cal-itp/benefits/issues \n"
"POT-Creation-Date: 2023-12-14 21:35+0000\n"
"POT-Creation-Date: 2024-01-19 00:29+0000\n"
"Language: Español\n"
"MIME-Version: 1.0\n"
"Content-Type: text/plain; charset=UTF-8\n"
Expand All @@ -33,7 +33,7 @@ msgstr ""
msgid "Cal-ITP Benefits"
msgstr "Cal-ITP Benefits"

msgid "Skip to Main Content"
msgid "Skip to main content"
msgstr "Saltar al contenido principal"

msgctxt "image alt text"
Expand Down
18 changes: 13 additions & 5 deletions benefits/static/css/styles.css
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,16 +284,24 @@ main {
left: unset !important;
transform: none;
overflow: hidden;
background: var(--focus-color);
font-size: var(--font-size-15px);
line-height: var(--bs-body-line-height);
letter-spacing: calc(var(--font-size-15px) * var(--letter-spacing-5));
}

#skip-to-content:focus {
height: 21px !important;
height: auto;
outline: none !important;
box-shadow: none;
padding: 1rem 0;
background: var(--bs-white);
}

#skip-to-content:focus span {
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.

font-size: var(--bs-body-font-size);
font-weight: var(--bold-font-weight);
letter-spacing: var(--letter-spacing-5);
line-height: var(--bs-body-line-height);
padding: 0 calc(4rem / 16);
}

/* Footer */
Expand Down
Loading