-
Notifications
You must be signed in to change notification settings - Fork 0
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
SUM-253 | add heading size option #66
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
8bd6478
to
d5bda52
Compare
const headerTag = headerTagChoice[0] | ||
const headerClasses = headerTagChoice[1]?.replace(".", " ").replace("su-font-splash", "type-3 mb-12") || "mb-12" | ||
const headerClasses = twMerge( |
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 greatly increasing complexity, I think we advise against this in the future.
@rebeccahongsf see this comment first since it might push you one way or another here. |
Co-authored-by: pookmish <[email protected]>
Co-authored-by: pookmish <[email protected]>
Co-authored-by: pookmish <[email protected]>
Co-authored-by: pookmish <[email protected]>
Co-authored-by: pookmish <[email protected]>
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.
just a small change, but I think this is better already
Co-authored-by: pookmish <[email protected]>
Co-authored-by: pookmish <[email protected]>
Co-authored-by: pookmish <[email protected]>
@@ -66,9 +66,11 @@ const PillCard = ({imageUrl, imageAlt, videoUrl, isArticle, children, bgColor, . | |||
|
|||
<div | |||
className={twMerge( | |||
"rs-px-3 flex flex-col pb-[125px] pt-20 @7xl:rs-px-4 @2xl:pb-[175px] @3xl:pb-[225px] @4xl:pb-[300px]", | |||
"rs-px-3 flex flex-col pb-[125px] pt-20 @7xl:rs-px-4 @2xl:pb-[175px] @3xl:pb-[225px] @4xl:pb-[300px] [&_p>a]:text-black-true hocus:[&_p>a]:text-[#00365C]", |
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.
Slipping in the accessibility color change as well 🤔 Does this darker digital blue color change work for the hover/focus state?
https://webaim.org/resources/contrastchecker/?fcolor=00365C&bcolor=F9A44A
https://webaim.org/resources/contrastchecker/?fcolor=00365C&bcolor=A6B168
https://webaim.org/resources/contrastchecker/?fcolor=00365C&bcolor=F4795B
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.
Looks like it does, but the links on the "Spirited Light" (2.46) and the "Olive Light" (2.86) still don't work
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.
Gotta fix the link colors in two card variants.
https://github.com/SU-SWS/summer-nextjs/pull/66/files#r1765795628
I went with an even darker blue 👀 Does that suffice or shall I go back to the drawing board 🤔 https://webaim.org/resources/contrastchecker/?fcolor=001829&bcolor=F9A44A |
READY FOR REVIEW
Summary
Review By (Date)
Review Tasks
Setup tasks and/or behavior to test
Associated Issues and/or People