Task/HOM-1 update white hero banner text color #826
Conversation
|
@shc314 I very much appreciate you opening this PR - text color change is great. The moving of styles look good also - I believe there are 4 more that needs to be moved (for completeness 😄 ). There are 2 lines that I'll call out in the comment because they're in the code diff, the other 2 I'll let you find them. |
| @@ -80,11 +74,9 @@ const Hero = ({ | |||
| > | |||
| <FlexBox direction="col" className="lg:su-mt-[19rem]"> | |||
There was a problem hiding this comment.
Let's move this to styles.js also 😄 @shc314
| blackText | ||
| )} | ||
| > | ||
| <div className={styles.scroll({ blackText })}> | ||
| <p className="su-mb-02em">Scroll to explore</p> |
There was a problem hiding this comment.
And here's another one. There are a couple more in this file.
There was a problem hiding this comment.
I hadn't moved them all because I wasn't sure what to call some of the variables. 😆 I think I got them all now. Please let me know if any of the names aren't clear.
| export const textWrapper = ({ blackText }) => | ||
| dcnb('su-text-center su-text-white', blackText); |
There was a problem hiding this comment.
There's probably a slightly better way to handle this. Here if blackText exists, you're going to have both of these classes su-text-white and su-text-black-true, and you're counting on su-text-black-true being compiled after su-text-white in the CSS so it overrides su-text-white. A better way would be to have a isBlackText variable as boolean instead, then you can pass in dcnb('su-text-center', isBlackText ? 'su-text-black-true' : 'su-text-white'), which is a lot safer @shc314
There was a problem hiding this comment.
Thank you for this suggestion. The text needs to be white when the screen size is smaller than the xs breakpoint. That's why I was using xs:su-text-black-true. How can I add a condition for smaller than the xs breakpoint?
There was a problem hiding this comment.
I see what you're doing - let me take a look later today @shc314
| @@ -99,13 +98,13 @@ const Hero = ({ | |||
| </div> | |||
| )} | |||
| {numCta > 0 && ( | |||
| <div className={sansSub ? 'su-rs-mt-4' : ''}> | |||
There was a problem hiding this comment.
@yvonnetangsu I wasn't sure if this one would still work if moved to hero.styles. Is sansSub referring to something different here?
There was a problem hiding this comment.
@shc314 You'll need to pass sansSub into the styles here like how you pass blackText into the other ones 😄 Otherwise in the styles file, it wouldn't know whether this field exists.
There was a problem hiding this comment.
Thank you. There was already a sansSub in the styles file, so I think that's why it didn't cause an error, but I don't think it's the same one as what's here.
| @@ -37,7 +37,7 @@ export const sansSub = ({ blackText }) => | |||
| 'xs:su-text-shadow-white': blackText === 'xs:su-text-black-true', | |||
| } | |||
| ); | |||
| export const marginTop = sansSub ? 'su-rs-mt-4' : ''; | |||
| export const marginTop = ({ sansSub }) => (sansSub ? 'su-rs-mt-4' : ''); | |||
There was a problem hiding this comment.
@yvonnetangsu After I passed sansSub as a prop, ESLint says 'sansSub' is already declared in the upper scope on line 33. Should I change the name of the one on line 33?
There was a problem hiding this comment.
@shc314 So yes, the fastest way to do this would be to update the variable name on line 33, but after that you'll need to update it in the hero.js file as well (where it uses styles.sansSub), so for example, if you rename the variable to export const subhead = 'something', then in hero.js, you'll use className={styles.subhead} 😄
There was a problem hiding this comment.
Thank you. I've changed the variable name and there is no error now.
There was a problem hiding this comment.
Thank you for the update @shc314! I'll get back to you next week on this - it's been a really busy week.
READY FOR REVIEW
Summary
su-text-black-trueReview By (Date)
Review Tasks
Associated Issues and/or People
@William-Misiaszek
https://internal-saa.atlassian.net/browse/HOM-1