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

Update header and footer, including new MIT logo #145

Merged
merged 3 commits into from
Jan 4, 2024
Merged

Conversation

matt-bernhardt
Copy link
Member

@matt-bernhardt matt-bernhardt commented Nov 30, 2023

This makes the branding and logo changes that are being rolled out across the Libraries' web platforms. it also picks up some fixes to broken markup that were revealed during a11y evaluation.

Ticket: https://mitlibraries.atlassian.net/browse/PW-72

Developer

Stylesheets

  • Any theme or plugin whose stylesheets have changed has had its version
    string incremented.

Secrets

  • All new secrets have been added to Pantheon tiers
  • Relevant secrets have been updated in Github Actions
  • All new secrets documented in README
  • No secrets are affected by this change

Documentation

  • Project documentation has been updated
  • No documentation changes are needed

Accessibility

  • ANDI or Wave has been run in accordance to
    our guide and
    all issues introduced by these changes have been resolved or opened as new
    issues (link to those issues in the Pull Request details above)

Stakeholder approval

  • Stakeholder approval has been confirmed
  • Stakeholder approval is not needed

Dependencies

NO dependencies are updated

Code Reviewer

  • The commit message is clear and follows our guidelines
    (not just this pull request message)
  • The changes have been verified
  • The documentation has been updated or is unnecessary
  • New dependencies are appropriate or there were no changes

@matt-bernhardt matt-bernhardt force-pushed the pw-72 branch 2 times, most recently from a13765c to 7992266 Compare December 14, 2023 16:14
** Why are these changes being introduced:

* MIT has released an updated logo, which we need to start implementing
  across our web platforms.

* While doing this, there are some additional changes that we've decided
  to make to clean up our footer content, and to move shared assets like
  these logos to our CDN.

** Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/pw-72

** How does this address that need:

* This replaces the old MIT logo (which were inline SVG markup) with the
  new logo (which is loaded via img tag from the CDN).

* The simple favicon.ico file is removed, and a more robust set of icons
  loaded from the CDN.

* The footer is cleaned up, with some updated links and the removal of
  social media icons.

* Stylesheets for the header, footer, and menu are updated to pay a bit
  more attention to logo sizes, and establish the relevant size
  constraints.

* Style rules for markup which is no longer present have been removed.
  I hope I got it all, but am not sure.

* Because the parent theme styles are changing, the theme version is
  updated to help with cache clearing on deploy.

** Document any side effects to this change:

* The final column of links in the mega-footer has more complex markup,
  because one of the elements now has two different links (so a class on
  an anchor tag is no longer sufficient). To stay consistent, I've made
  this change (having the class on a span, which then contains the link)
  on all items in that column.

* Some of the responsiveness of the header is a bit wonky, and probably
  in need of a wholesale rebuilding of the stylesheets in the future.
  UX has signed off on how it behaves, though, and starting the rebuild
  from the WordPress implementation is the wrong way to tackle that
  issue - so I'm leaving it as is for now.
While doing a11y testing on the header and footer, I noticed
some markup bugs getting flagged that were really easy to
fix; this commit has those.
Copy link

@jazairi jazairi left a comment

Choose a reason for hiding this comment

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

Fwiw, this bit of CSS is why the account/chat links in the nav align weirdly. Those elements need to be centered columns because in larger viewports, they have the graphic that sits above. In smaller viewports, though, it looks weird because the main axis is swapped, so align-items: flex-end doesn't have the same effect as flex-direction: row.

My fix for that was to override the media query at max-width: 1023px to add flex-direction: row and align-items: flex-end to .nav-main .small .a. This is one of the weird cases where it may be harder to work with Sass than compiled CSS, but it seems like the corresponding mixin is bp-tablet--landscape-below. In that case, I think this is the corresponding rule.

All that said, this is already approved by stakeholders, so I wouldn't hold it against you if you'd rather leave well enough alone. 🙂

The a11y fixes look good as well. Those were the ones that stood out to me from the validation. It doesn't feel great to remove an alt attribute without replacing it, but my initial research on the subject indicates that ARIA is the solution. Given that we are both terrified of mis-applying ARIA attributes, I'm happy to leave it as is.

@matt-bernhardt
Copy link
Member Author

Coming back to this after the year-end break, I find myself perilously close to falling into the same rabbit hole that's consumed us in December here. I think you're on to something, though - and the two rules you're suggesting seem to make things better. Thanks for digging into this a bit more.

I'm going to push a commit to make these tweaks, and will proceed with deploying this tomorrow.

One of the takeaways on this whole rebranding effort is the need for clear documentation for future-us to understand how our clever CSS approaches work, for the next time we need to make changes like this.

Copy link

@jazairi jazairi left a comment

Choose a reason for hiding this comment

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

The change appears to be in the right place, assuming I understand the media query mixins. I haven't checked locally yet.

I was looking back through all the changes and noticed a discrepancy between the heights/widths of your logos and what I have in Springshare. I don't remember which is correct, but even if these are a few px off of what we decided on, I don't think a change is required or appropriate because this has already been approved by stakeholders. I just want to make sure I don't have it wrong on my end, since the Springshare stuff hasn't gotten final approval yet.

web/app/themes/mitlib-parent/footer.php Show resolved Hide resolved
web/app/themes/mitlib-parent/header.php Show resolved Hide resolved
@matt-bernhardt matt-bernhardt merged commit 19eff35 into master Jan 4, 2024
3 checks passed
@matt-bernhardt matt-bernhardt deleted the pw-72 branch January 4, 2024 00:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants