-
Notifications
You must be signed in to change notification settings - Fork 38
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
feat: update the paragon theme with new branding #9
Conversation
Following up from our Slack conversation, it looks the
We could override the theme to address the |
Should the Button with See https://paragon-edx-next.netlify.app/components/button. Actually, similar question on a11y color contrast ratio for all the button variants; looks like |
.btn-link { | ||
text-decoration: $link-decoration; | ||
text-decoration: none; |
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.
Is it intentional for .btn-link to not be underlined? In the past, we've been asked to underline .btn-link elements by UX and/or a11y since it doesn't have it by default. But this request hasn't always been consistent.
I'm game for either case, just that we're consistent about it :)
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.
This is intentional. Non-underlined links buttons are usually sitting next to a regularly styled one. My understanding is that in the context of a "nav" or place where it should be obvious that they're interactive the underline is unneeded
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.
👍 Got it. Yeah, in the past, we've been asked to underline link buttons that sit next to a regularly styled one.
We worked with @wittjeff on this, the contrast requirement doesn't apply to the borders. |
README.rst
Outdated
@@ -8,12 +8,22 @@ This project contains branding assets and themes for edx.org. It is the edX impl | |||
Usage | |||
----- | |||
|
|||
Install this package: | |||
Install this package one of two ways |
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.
Can we add any guidance here on why you might prefer one or the other?
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.
Added!
@@ -68,7 +75,7 @@ | |||
color: $body-color; | |||
font-weight: 400; | |||
&:before { | |||
background-image: $icon; | |||
background-image: str-replace($icon, "#", "%23"); |
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.
wut
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.
It's a weird issue. You need the #
in the string template where $icon
is defined in order to pipe the color into the url encoded svg, but then the #
needs to actually be url encoded.
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.
LGTM, though the alert styling without the borders is lower-contrast and hard to see, fading into the white background for me.
(might be affected by my being somewhat color-blind)
🎉 This PR is included in version 1.1.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@abutterworth I was referring to the button text colors, not the borders. |
Oh sorry yea. That button styling is for use on dark backgrounds. Today's light button is similar I think. We shouldn't use it at a rule though. I need to add a set of buttons for our dark palette |
See the theme in this PR applied to the Paragon docsite here: https://paragon-edx-next.netlify.app/
Notable changes:
$base-font-size
is now 18px instead of 16px.variant="brand"
orvariant="outline-brand"