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

Use Smoot Design lib in MIT Learn #1979

Merged
merged 14 commits into from
Jan 23, 2025
Merged

Use Smoot Design lib in MIT Learn #1979

merged 14 commits into from
Jan 23, 2025

Conversation

ChristopherChudzicki
Copy link
Contributor

@ChristopherChudzicki ChristopherChudzicki commented Jan 21, 2025

What are the relevant tickets?

https://github.com/mitodl/hq/issues/6279

Description (What does it do?)

This PR begins using components from https://github.com/mitodl/smoot-design, in particular:

  • ThemeProvider, Button, ActionButton, ButtonLink, ActionButtonLink

now all come from @mitodl/smoot-design.

Review alongside mitodl/smoot-design#40

Screenshots (if appropriate):

There should be no visible changes, with a small exception mentioned below in a comment.

How can this be tested?

  1. Check the site. Buttons should work and should look the same.
  2. Also check some ButtonLink instances. These should be using NextJS's Link component via the theme's LinkAdapter.
    • An example is the "My Dashboard" link in "Personalize" section of homepage. If you click it with the JS console open, it should go to your dashboard AND you'll know it's a NextJS link b/c the JS console isn't cleared upon navigation.

Additional Context

There are a few more components that we could use directly from smoot-design, but this PR is big enough already so those can be handled separately.

Checklist

  • replace git-installed version of smoot-design

@ChristopherChudzicki ChristopherChudzicki changed the title Cc/smoot design Use Smoot Design lib in MIT Learn Jan 21, 2025
@ChristopherChudzicki
Copy link
Contributor Author

The two (small) visible changes that are expected in this PR are

In both cases, the changed version matches figma. Some extra button variants had been accidentally introduced and reused.

Screenshot 2025-01-21 at 4 36 39 PM Screenshot 2025-01-21 at 4 38 41 PM

@ChristopherChudzicki ChristopherChudzicki marked this pull request as ready for review January 21, 2025 21:46
@gumaerc gumaerc self-assigned this Jan 22, 2025
Copy link
Contributor

@gumaerc gumaerc left a comment

Choose a reason for hiding this comment

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

Just a few questions, but an overall smoke test of the site and the code is looking good:

// Picked via figma eyedropper.
// Figma specifies multiple backgrounds, background: linear-gradient(...), brightRed
// But that causes problems for the transition animation
backgroundColor: "#FCF2F4",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we add this color to the theme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For now at least, I don't think so. It's very one-off.

It's the hover color of secondary buttons, specified in figma as

background: linear-gradient(0deg, rgba(255, 255, 255, 0.94) 0%, rgba(255, 255, 255, 0.94) 100%), #D02E44;

I.e., #D02E44 (brightRed) overlaid with a white, slightly transparent background.

As mentioned in the comment, the background caused trouble with CSS transitions (color fading over course of 250ms), so I had to use an actual color rather than overlaid backgrounds.


Why is this here in the first place? MIT Learn and smoot-design repo both implement our variant="secondary" buttons as transparent background plus a slightly red transparent effect on hover.

  • According to figma, it should actually be a white background with a similar hover effect.
  • We should fix smoot-design to align with figma, but I'd like to handle that separately.

@@ -14,7 +14,7 @@
"@emotion/cache": "^11.13.1",
"@emotion/styled": "^11.11.0",
"@mitodl/course-search-utils": "3.3.2",
"@mitodl/smoot-design": "^0.0.0-preview215f7ae3fa",
"@mitodl/smoot-design": "https://github.com/mitodl/smoot-design.git#35aa22f9cb5e687bff63048d132dcb7288bcf526",
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason this dependency is pulled directly from Github? Are we not publishing this library to NPM?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's in the checklist ! Once mitodl/smoot-design#40 is approved /released.

Copy link
Contributor

@gumaerc gumaerc 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 to merge based on the comments above. Remember to point the dependency at the new commit of smoot-design before merging.

@ChristopherChudzicki ChristopherChudzicki merged commit 63c0b7b into main Jan 23, 2025
11 checks passed
@odlbot odlbot mentioned this pull request Jan 24, 2025
7 tasks
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