-
Notifications
You must be signed in to change notification settings - Fork 36
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
Conversation
There was a problem hiding this 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
beautiful! thank you :)
There was a problem hiding this 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; |
There was a problem hiding this comment.
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
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