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

DOP-4623: Dark mode for Cards #1114

Merged
merged 12 commits into from
Jun 10, 2024
Merged

DOP-4623: Dark mode for Cards #1114

merged 12 commits into from
Jun 10, 2024

Conversation

mayaraman19
Copy link
Collaborator

@mayaraman19 mayaraman19 commented Jun 5, 2024

Stories/Links:

DOP-4623

Current Behavior:

Current docs compass landing page
Current drivers landing page

Staging Links:

Docs compass (built off of this PR)
Drivers landing page (built off of this PR)

Notes:

Figma

Upgrades Card component to support Dark Mode, as well as adds functionality to either get the card icon from the version stored in the content repo, or get it from the Web team's URL (which pulls from this list), which enables easier access to dark mode images since we can just add _inverse instead of having to include both the light mode and dark mode versions of the images. See Parser PR here.

The driver cards in the docs-landing IA, while written as card directives in the rST, are actually SideNavItems so I will leave those out of the scope of this ticket.

Additional work should probably involve having a fallback image when the inverse image doesn't exist.

README updates

    • This PR introduces changes that should be reflected in the README, and I have made those updates.
    • This PR does not introduce changes that should be reflected in the README

@mayaraman19 mayaraman19 changed the title Dop 4623 b DOP-4623: Dark mode for Cards Jun 5, 2024
@mayaraman19 mayaraman19 requested review from seungpark and mmeigs June 6, 2024 13:14
@mayaraman19 mayaraman19 marked this pull request as ready for review June 6, 2024 13:15
Copy link
Collaborator

@caesarbell caesarbell left a comment

Choose a reason for hiding this comment

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

I left a small (nit) suggestion, but overall LGTM. Thank you for working on this and leaving a very descriptive description in the PR.

@@ -155,11 +157,26 @@ const Card = ({
isLanding && !isLargeIconStyle ? landingStyles : '', // must come after other styles to override
];

let iconSrc;
Copy link
Collaborator

Choose a reason for hiding this comment

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

(nit) I wonder if by reducing the nested if statements and using ternary operators, we could make the code cleaner and more readable. Maybe something like;

let iconSrc;

if (icon) {
  const isPath = icon.includes('/');
  const getIcon = `${icon}${darkMode ? '_inverse' : ''}`;
  const webImageURL = `https://webimages.mongodb.com/_com_assets/icons/${getIcon}.svg`;

  iconSrc = isPath
    ? (darkMode && iconDark ? withPrefix(iconDark) : withPrefix(icon))
    : webImageURL;
}

If you don't agree, please feel free to ignore it as this is not a blocker.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

beautiful! thank you :)

Copy link
Collaborator

@mmeigs mmeigs 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!! I wish our icons had better resolution... 👀 but not your problem!

border-radius: 24px;
font-family: 'Euclid Circular A',Akzidenz,'Helvetica Neue',Helvetica,Arial,sans-serif;
Copy link
Collaborator

Choose a reason for hiding this comment

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

omg thank god, get that dark-sided name outta here

@mayaraman19 mayaraman19 merged commit 2fe6423 into main Jun 10, 2024
2 checks passed
@mayaraman19 mayaraman19 deleted the DOP-4623-b branch June 10, 2024 21:09
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.

3 participants