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

upgrade stories & events grid to grid2 #1792

Merged
merged 1 commit into from
Nov 5, 2024
Merged

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Nov 5, 2024

What are the relevant tickets?

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

Description (What does it do?)

This PR fixes a bug that was introduced when MUI was updated from v5 to v6 as part of #1776.

MUI v5 had Grid and Unstable_Grid2 components. In v6, Grid2 was stabilized Grid was deprecated. Additionally, v6 seems to have introduced a bug in Grid. The bug only affects Grid usage that has string-values for columnSpacing. We had exactly one instance of string-valued columnSpacing. So I've upgraded that particular usage to Grid2 and everything looks fine now.

See mui/material-ui#44321 for MUI bug.

How can this be tested?

  1. On this branch, compare styles of Stories & Events section of the homepage with that on https://learn.mit.edu/. (The bug has not yet made it to production). When comparing styles, I recommend:
    • Put local branch in browser Tab 1, production in Tab 2
    • Ensure zoom level is same in both tabs
    • Ensure both tabs have same widths (e.g., close dev tools on both tabs)
    • swap between Tab 1 and Tab 2.
  2. Suggest doing additional style comparison of other pages, e.g. /departments, /topics, /units, /search, /dashboard + its tabs, and channel pages.
    • when comparing, ensure both tabs have scrollbar or both tabs do not have scrollbar. E.g., if viewing /dashboard/my-lists, if one tab has 1 list and the other has 10 lists, scrollbar might affect positioning in the longer list.

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review November 5, 2024 15:06
@ChristopherChudzicki ChristopherChudzicki added the Needs Review An open Pull Request that is ready for review label Nov 5, 2024
Copy link
Contributor

@jonkafton jonkafton left a comment

Choose a reason for hiding this comment

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

Looks good / fixed. Instances of Grid v1 also good.

@ChristopherChudzicki ChristopherChudzicki merged commit 735cef4 into main Nov 5, 2024
11 checks passed
This was referenced Nov 5, 2024
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