Skip to content

Remove some unused exports from ol-components #1844

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

Merged
merged 6 commits into from
Nov 25, 2024

Conversation

ChristopherChudzicki
Copy link
Contributor

What are the relevant tickets?

For https://github.com/mitodl/hq/issues/6039

Description (What does it do?)

This cleans up ol-components mui re-exports a bit:

  • Removes some exports we simply were not using
  • Removes a few exports that were very rarely used and whose usage was easily replaced (example: Box)
  • Removes MuiCard and friends. This is still used in ol-widgets, where it's just imported from mui directly.

How can this be tested?

  1. Visit /c/unit/ocw or any other channel page. The subscribe button should be in same spot.
  2. Visit /search?q=gdjslkghafkaghafdklhdafsbk the "Not found" card should look the same
  3. Visit /kjgoidsh the "Not Found" error page should look the same.

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review November 21, 2024 20:09
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.

Nice tidy up!

Comment on lines +10 to +17
const ErrorContainer = styled(Container)(({ theme }) => ({
backgroundColor: theme.custom.colors.white,
borderRadius: "8px",
marginTop: "4rem",
padding: "16px",
boxShadow:
"1px 2px 1px -1px rgba(0,0,0,0.2),0px 1px 1px 0px rgba(0,0,0,0.14),0px 1px 3px 0px rgba(0,0,0,0.12)",
}))
Copy link
Contributor

Choose a reason for hiding this comment

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

Are these part of the design system and could be their own component / card variant or are they left over from before the Smoot treatment or just default Mui styles?

The error page and no search results may be the only instances.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The box shadow there is a style from MuiCard.

Rather than importing MuiCard in main, I just copied the boxshadow to preserve the existing behavior.

</Stack>
{channel.data ? <ChannelDetails channel={channel.data} /> : null}
</BannerContent>
</Container>
</StyledBannerBackground>
<Container component="section">
<Container id="foo" component="section">
Copy link
Contributor

Choose a reason for hiding this comment

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

foo for testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep. Oops

@ChristopherChudzicki ChristopherChudzicki merged commit 0e4c18e into main Nov 25, 2024
11 checks passed
@odlbot odlbot mentioned this pull request Nov 25, 2024
19 tasks
mbertrand pushed a commit that referenced this pull request Dec 2, 2024
* chore: remove unused exports

* remove muidialog from ol-components

* remove box usage

* remove mui cards

* fix a test

* remove testing id
@rhysyngsun rhysyngsun deleted the cc/cleanup-ol-components branch February 7, 2025 20:41
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