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

Make sign-in UI consistent (IE11 rendered poorly) #392

Merged
merged 3 commits into from May 19, 2020
Merged

Make sign-in UI consistent (IE11 rendered poorly) #392

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
41168bb
Make sign-in UI consistent (IE11 rendered poorly)
slj May 8, 2020
1a7525a
Moved sign-in button CSS background to style tag
slj May 8, 2020
66d7c51
Moved style back to CSS + replaced id with class
slj May 8, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 6 additions & 7 deletions _includes/header.html
View file
Open in desktop
Expand Up @@ -88,19 +88,18 @@
{% endfor %}
</ul>
</li>

</ul>
</div>
<div class="usa-nav__secondary">
<div class="usa-nav__secondary-container">
<ul class="usa-nav__secondary-links">
<li class="usa-nav__secondary-item bold">
<li id="sign-in" class="usa-nav__secondary-item bold">
<span class="font-sans-md">{% t nav.sign_in %}</span>
<button tabindex="-1" class="usa-button--small sign-in-logo sign-in-logo-padding">
<a href="{% if site.lang != 'en' %}https://secure.login.gov/{{site.lang}}/{% else %}https://secure.login.gov/{% endif %}" title="Sign In" aria-label="Sign in">
<img src="{{ site.baseurl }}/assets/img/logo_sign_in.svg" alt="login.gov">
</a>
</button>
<span class="usa-button--small sign-in-logo sign-in-logo-padding">
<a href="https://secure.login.gov/{% if site.lang != 'en' %}{{site.lang}}{% endif %}"
style="background: url({{ site.baseurl }}/assets/img/logo_sign_in.svg) center/contain no-repeat"
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like it's generally an anti-pattern to do direct style attributes in HTML for things we can manage in the stylesheets.

Was the reason for doing this to get the asset URL correct? If so, it looks like other background URLs in our stylesheets seem to do relative URLs, ex url(../img/alerts/warning.svg), have we given that a shot?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for pointing this out - I originally had this rule in a stylesheet but the url path was incorrect. I should have looked through the other files for a correct example. I will revert/correct this change. I agree with you that inline styles are best avoided when possible.

title="Login.gov" aria-label="Login.gov" tabindex="0">Login.gov</a>
</span>
</li>
</ul>
<form class="usa-search usa-search--small" id="search_form" action="https://search.usa.gov/search" accept-charset="UTF-8" method="get"><input name="utf8" type="hidden" value="&#x2713;" />
Expand Down
28 changes: 26 additions & 2 deletions _sass/components/_nav.scss
View file
Open in desktop
Expand Up @@ -4,16 +4,40 @@
}
}

#sign-in {
Copy link
Contributor

Choose a reason for hiding this comment

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

one school of thought I've seen is that using IDs for styles is an anti-pattern because it means things are not repeatable. What do you think of making this like .sign-in-logo-wrapper or something as a class? In case we ever want a second button on the page?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As I see this element being unique in the long-term, I felt the id attribute was appropriate. If we ever needed to add another similar element, at that time we'd certainly change the id to a class, but I'm fine doing that now to save an extra step later.

display: flex;
align-items: center;
}

.sign-in-logo {
background-color: transparent;
width: 103px;
height: 27px;
padding: 6px;
border-color: map-get($site-palette, 'site-light-blue');
padding: 7px 6px 6px;
margin-left: 0.2rem;
border: 1px solid map-get($site-palette, 'site-light-blue');
display: flex;
justify-content: center;
align-items: center;

&:hover {
border-color: $blue;
}

a {
font-size: 1.28rem;
color: transparent;

&:hover {
color: transparent;
}
}
}

.usa-search [type=search] {
height: 2rem;
border-top-right-radius: 0;
border-bottom-right-radius: 0;
}

.logo {
Expand Down