Skip to content
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

updated header #1622

Open
wants to merge 15 commits into
base: main
Choose a base branch
from
Open

updated header #1622

wants to merge 15 commits into from

Conversation

gumaerc
Copy link
Contributor

@gumaerc gumaerc commented Sep 27, 2024

What are the relevant tickets?

Closes https://github.com/mitodl/hq/issues/5305

Description (What does it do?)

This PR implements the new site header as shown in the issue above and the screenshots below.

Screenshots (if appropriate):

image
image
image
image

How can this be tested?

  • Spin up mit-learn on this branch
  • Load the home page at https://localhost:8062/
  • Compare the new header to the Figma designs in the above issue both while logged in and logged out and make sure everything shows up as it should

@gumaerc gumaerc added Needs Review An open Pull Request that is ready for review and removed Work in Progress labels Sep 30, 2024
@shanbady shanbady self-requested a review September 30, 2024 17:26
@shanbady shanbady self-assigned this Sep 30, 2024
Copy link
Contributor

@shanbady shanbady left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There seems to be some extra padding around the header. Figma is showing 72p height but locally its coming out to 76px.

@gumaerc
Copy link
Contributor Author

gumaerc commented Sep 30, 2024

There seems to be some extra padding around the header. Figma is showing 72p height but locally its coming out to 76px.

@shanbady We actually had a discussion about this in Slack, but there seems to be an issue with Figma here. If you look at the box model for the header element in a browser, it is correctly including the bottom 4px border:

image

In Figma, the height is shown as "72px" because it's not including the 4px bottom border, even though it is shown in their box model:

image

I've confirmed with @mbilalmughal that the height is correct as-is.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have a mit-logo-learn.jpg which is used in subscription emails - https://learn.mit.edu/static/images/mit-logo-learn.jpg - are we leaving this as the old one for now or should this get swapped out as well (also worth noting it is intentionally a jpg for email template support)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Needs Review An open Pull Request that is ready for review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants