💄 DOP-4658 side nav now handles dark mode #1127
Conversation
|
could you add ability for the IALinkedData, aka the drivers cards in the sidenav, to support dark mode icons? This PR in the parser created the |
|
also, on selection of a sidenav item, it seems like in the Figma the text should be light green upon selection. From figma: |
@mayaraman19 I didn't work on those due to the ticket's description i.e. But if this has changed, I can gladly work on logos/cards within this ticket. cc @seungpark @mmeigs. |
I shared this with Liz to see if we want to stick with LG side nav designs or was the green font from Figma intentional. |
@seungpark, yeah, there were no designs in Figma for this, so I went with my best guess. I will share this with the design team and verify the assumptions with them. |
|
@mayaraman19 @seungpark, share the thread that contains the feedback we got from the design team. |
We decided to set up the IALinkedData component to handle |
src/components/Card/index.js
Outdated
|
|
||
| iconSrc = isPath ? (darkMode && iconDark ? withPrefix(iconDark) : withPrefix(icon)) : imageUrl; | ||
| } | ||
| let iconSrc = getSuitableIcon(darkMode, iconDark, icon); |
There was a problem hiding this comment.
nit: Can change this to a const now that it's a beautiful one-liner
| <div | ||
| css={css` | ||
| margin-left: ${hasChildren || isTocIcon ? '0px' : '21px'}; | ||
| color: ${isActive ? `${palette.green.dark3};` : `${palette.gray.dark3};`}; |
There was a problem hiding this comment.
Wondering about this change: is the color change and specificity not necessary anymore?
There was a problem hiding this comment.
@mmeigs good question, with the side nav upgrade it handles the active state color, and since the design team opted for matching the LG side nav styles rather than the Figma, we no longer needed to handle it anymore by applying our own style for the active state.
src/utils/get-suitable-icon.js
Outdated
| * @param {string} icon | ||
| * @returns {string} | ||
| */ | ||
| export const getSuitableIcon = (isDarkMode, iconDark, icon) => { |
There was a problem hiding this comment.
thanks so much for adding this and putting it into the Card component as well!
one super miniscule nit is that I feel like the order of the parameters would make more sense as icon, iconDark, isDarkMode. Not super sure why, maybe just feel like the icon parameter should come first. Obviously this is really tiny, though, so do whatever you want!
There was a problem hiding this comment.
@mayaraman19 good call on that, icon would be the prominent param, which I can see your case for it coming first. I will make the change. Thank you for your feedback on that.
Stories/Links:
DOP-4658
Current Behavior:
Docs Landing
Docs Manual
Docs Drivers Landing
Staging Links:
feature flag enabled
Staged: Docs Landing(sourced from a fork docs landing)
Staged: Docs Manual
Staged: Docs Drivers Landing (built off this pr)
We can also compare it to the stage page from @mayaraman19 , found here
feature flag disabled
Staged: Docs Landing
Staged: Docs Atlas
Notes:
In this PR, we updated the LG Side Nav component, along with tweaking some styles to support dark mode. Due to the LG Side Nav component upgrade, we were able to remove some custom styles. We also added the dark mode style to the
IALinkedDataComponent.README updates