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
Make sign-in UI consistent (IE11 rendered poorly) #392
Conversation
**Why**: LG-2963 **Why**: The UI to navigate to secure.login.gov was inconsistent **How**: Reduced markup to span > a for better accessibility **How**: Revised CSS
@slj On my Firefox (Developer) and Chrome browsers, the |
Thx for checking @nickttng. This is true for me too. Should be a simple fix. |
**Why**: url needs reference to {{ site.baseurl }} **Why**: {{ site.baseurl }} can't be referenced in CSS
_includes/header.html
Outdated
</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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
_sass/components/_nav.scss
Outdated
@@ -4,16 +4,40 @@ | |||
} | |||
} | |||
|
|||
#sign-in { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
**Why**: The relative path can be specified in the stylesheet **Why**: Using a classname will allow for multiple sign-in elements
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why: LG-2963
Why: The UI to navigate to secure.login.gov was inconsistent
How: Reduced markup to span > a for better accessibility
How: Revised CSS