-
Notifications
You must be signed in to change notification settings - Fork 3
Add marketing images to homepage testimonial, fix some styling issues #1077
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
Conversation
46ead5e to
39bbb3f
Compare
…e rather than use the avatar
- Moved the attestant info to its own component - Styling fixes for the homepage carousel - Limiting the homepage carousel to max 1440px per 4541
24f02d6 to
aec971e
Compare
for more information, see https://pre-commit.ci
….com:mitodl/mit-open into jkachel/4559-updates-to-testimonial-carousel
gumaerc
left a comment
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 functions well and solves all the problems in the issues. The spacings all match Figma, but the design should probably be reviewed by @steven-hatch and Bilal (Github won't tag him for me for some reason). I found a few things that I wanted to ask about:
| color: theme.custom.colors.mitRed, | ||
| fontStyle: "normal", | ||
| height: pxToRem(80), | ||
| fontWeight: theme.typography.fontWeightBold, |
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.
In typography.ts, we define 3 font weights that match the designs:
const fontWeights = {
text: {
roman: 400,
medium: 500,
bold: 700,
},
}Thus, the values you end up getting here come from the MUI defaults. In typography.ts, can we assign the MUI properties to our values specified here? For example:
const globalSettings: ThemeOptions["typography"] = {
// Note: Figma calls this "Neue Haas Grotesk Text", but that is incorrect based on Adobe's font family.
fontFamily: "neue-haas-grotesk-text, sans-serif",
fontWeightLight: fontWeights.text.roman,
fontWeightRegular: fontWeights.text.roman,
fontWeightMedium: fontWeights.text.medium,
fontWeightBold: fontWeights.text.bold,
...The MUI defaults don't end up being functionally different (MUI has 300 for light and we use 400) but using the values defined here will ensure that the values match the font we've chosen, in this case neue-haas-grotesk-text, sans-serif.
We actually have a linting rule that is supposed to disable manually specifying font-weight, but I think it was allowed here because it only disallows numeric weights. Anyway, this is definitely a special case as it's not really regular text and is effectively being used here as an icon. Maybe this whole thing would be better as an SVG, but that's another conversation.
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 basically just added the font weights into the theme options as specified. There are a couple other places where this gets used:
mit-open/src/pages/HomePage/NewsEventsSection.tsxol-components/src/components/ChoiceBox/ChoiceBox.tsx
The onboarding is broken for me locally at the moment, which is where ChoiceBox seems to be used exclusively, but the news and events section didn't seem to change.
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.
My point was more that the MUI font weight properties (i.e. theme.typography.fontWeightbold) are not currently customized by our theme, so when you call those properties now you are going to get the MUI defaults. For theme.typography.fontWeightLight for example, you get 300 back whereas in our case that should be 400. This recommendation is based on a conversation with @ChristopherChudzicki about how our specified font weights relate to the imported Abobe font.
| } | ||
|
|
||
| const generateMarketingImageSrc = () => { | ||
| const idx = Math.floor(Math.random() * 6) + 1 |
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 there something we can do here to ensure that two of the same images don't land directly next to each other? This happened to me and of course I didn't get a screenshot of it, but it didn't look great to have the exact same images on two cards... at least side by side.
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 added some code to do this - you will still get wraparound but it shouldn't drop one next to each other. The algorithm is pretty simple but can also set it up so that you don't get a repeat until all of them have been seen once.
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, that makes sense. We can always add more marketing images too... as long as we have more images than attestations then we'll never see a repeat.
…ake sure the marketing images don't appear twice in a row
gumaerc
left a comment
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
What are the relevant tickets?
Closes mitodl/hq#4559
Closes mitodl/hq#4541
Description (What does it do?)
Screenshots (if appropriate):
Regular desktop:

Extra-wide desktop:

Mobile:

How can this be tested?
Automated tests should pass. This includes some new tests for the testimonial sliders, so those specifically should pass - they're in
HomePage.test.tsxandTestimonialDisplay.test.tsx.The layout should more closely match what's in Figma now, and the cover image on the left should be one of the six marketing images. This just chooses one at random, it makes no effort to check to see if one's already been chosen or not.
For the unit page testimonials: the attestant block is now its own component - this should have no real change on the unit page sliders but it is a change.