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

refactor(nav-drawer): update themes #1167

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

desig9stein
Copy link
Contributor

@desig9stein desig9stein commented Apr 18, 2024

closes #1152

@@ -38,13 +38,13 @@ const light = {

const dark = {
shared: css`
${sharedDark}
${sharedLight}
Copy link
Contributor

Choose a reason for hiding this comment

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

It can be called just shared, if it's shared between both light and dark

@@ -2,9 +2,14 @@
@use '../../light/themes' as *;

$theme: $bootstrap;
$background: var-get($theme, 'background') !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't need these variables here, as you're not using them.

background: var-get($overlay-fluent, 'background-color');
}
}
$background: var-get($theme, 'background') !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need these variables, nor this file. You're not overwriting anything.

background: var-get($overlay-indigo, 'background-color');
}
}
$background: var-get($theme, 'background') !default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my previous comment

@use 'styles/utilities' as *;
@use '../../light/themes' as *;

$theme: $material;
Copy link
Contributor

Choose a reason for hiding this comment

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

it's fluent, but you don't even need this variable as you're not getting anything from the theme.

@use 'styles/utilities' as *;
@use '../../light/themes' as *;

$theme: $material;
Copy link
Contributor

Choose a reason for hiding this comment

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

same as my previous comment

@use 'styles/utilities' as *;
@use '../../light/themes' as *;

$theme: $material;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as my previous comment. Remove all $theme variables where they're not being used.

@didimmova
Copy link
Contributor

Comments are still not resolved.

- Delete nav-drawer.fluent.scss and nav-drawer.indigo.scss
@imincheva

This comment was marked as resolved.

@AnjiManova

This comment was marked as resolved.

@sbayreva

This comment was marked as resolved.

@simeonoff simeonoff requested a review from didimmova May 16, 2024 14:34
@imincheva

This comment was marked as resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Align navdrawer implementation with UI kit designs
7 participants