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-4713: implement DarkMode Dropdown in new Action Bar #1105

Merged
merged 13 commits into from
Jun 4, 2024
Merged

Conversation

seungpark
Copy link
Collaborator

@seungpark seungpark commented May 31, 2024

Stories/Links:

DOP-4713

Staging Links:

drivers site staged change with new Action Bar
docs landing with flag turned off

Notes:

  • As noted in previous dark mode PR's, test locally with GATSBY_ENABLE_DARK_MODE=true
  • Discussed offline but noting that the state from DarkModeContext has been updated from a boolean to a string type to accommodate for system selection (default) case.
  • Includes changes for Action Bar set to contain new feedback and chatbot search bar
  • Added LG/menu to our packages to use for DarkMode Dropdown
  • Follow up PR will include unit tests to test new Context and new DarkModeDropdown

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

@seungpark seungpark marked this pull request as ready for review May 31, 2024 19:04
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.

Nits below.

Also, I believe that there's a bug somewhere for the dark mode display of the Menu itself. In the figma, it shows a light mode menu and a dark menu, but I can only get the dark menu to show up for me no matter what setting I'm on.

@@ -11,7 +11,7 @@ const StyledHeaderContainer = styled.header(
grid-area: header;
top: 0;
z-index: ${theme.zIndexes.header};
${props.template === 'landing' ? '' : 'position: sticky;'}
${props.template === 'landing' || process.env['GATSBY_ENABLE_DARK_MODE'] === 'true' ? '' : 'position: sticky;'}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does this mean that we don't want the header to stick to the top when scrolling on dark mode?

Copy link
Collaborator

Choose a reason for hiding this comment

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

It looks great, so I feel like I must have just missed a design discussion!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

i believe so! this was discussed Design Team in this thread

src/context/dark-mode-context.js Outdated Show resolved Hide resolved
src/context/dark-mode-context.js Outdated Show resolved Hide resolved
Comment on lines +55 to +58
<Wrapper newChatbotLanding={process.env['GATSBY_ENABLE_DARK_MODE'] !== 'true' && useChatbot}>
{process.env['GATSBY_ENABLE_DARK_MODE'] !== 'true' && useChatbot && (
<ChatbotUi template={pageContext?.template} />
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure I understand this. Are we only using the Chatbot when dark mode is enabled?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

the intention here was that the Chatbot would be included in the new ActionBar, not here when enable_dark_mode is enabled

Comment on lines 12 to 19
const IconStyling = css`
cursor: pointer;
`;

const MenuStyling = css`
width: fit-content;
margin-top: ${theme.size.small};
`;
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: Our conventions are to capitalize the first letter only when they are styled components. When they are styles injected in classnames we keep the first letter lowercase.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

thanks for pointing this out! refreshed

@seungpark
Copy link
Collaborator Author

Also, I believe that there's a bug somewhere for the dark mode display of the Menu itself. In the figma, it shows a light mode menu and a dark menu, but I can only get the dark menu to show up for me no matter what setting I'm on.

I believe it's happening but its very subtle! Figma shows the following colors for dark/light mode:

dark:

image

light:

image

which correspond to the menu colors:
dark:

image
llight:

image

@seungpark seungpark requested a review from mmeigs June 4, 2024 14:36
@mmeigs
Copy link
Collaborator

mmeigs commented Jun 4, 2024

Also, I believe that there's a bug somewhere for the dark mode display of the Menu itself. In the figma, it shows a light mode menu and a dark menu, but I can only get the dark menu to show up for me no matter what setting I'm on.

I believe it's happening but its very subtle! Figma shows the following colors for dark/light mode:

This might be another time I'm looking at old news, but I was just referencing the light and dark mode dropdowns on the left side of the Figma

@mmeigs
Copy link
Collaborator

mmeigs commented Jun 4, 2024

Also, I believe that there's a bug somewhere for the dark mode display of the Menu itself. In the figma, it shows a light mode menu and a dark menu, but I can only get the dark menu to show up for me no matter what setting I'm on.

I believe it's happening but its very subtle! Figma shows the following colors for dark/light mode:

This might be another time I'm looking at old news, but I was just referencing the light and dark mode dropdowns on the left side of the Figma

Worked out offline. Seung is right. Matt is wrong. LG is slightly psycho.

@seungpark seungpark merged commit ca434bb into main Jun 4, 2024
2 checks passed
@seungpark seungpark deleted the DOP-4713 branch June 4, 2024 18:36
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