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

Initial Conversion of SCSS to CSS #5375

Open
wants to merge 24 commits into
base: main
Choose a base branch
from
Open

Initial Conversion of SCSS to CSS #5375

wants to merge 24 commits into from

Conversation

Zystix
Copy link
Contributor

@Zystix Zystix commented Dec 13, 2024

Why

Converting SCSS to CSS

https://cultureamp.atlassian.net/browse/KZN-2877

What

Converts Avatar, AvatarGroup, Badge, Brand, BrandMoment to CSS

Copy link

changeset-bot bot commented Dec 13, 2024

🦋 Changeset detected

Latest commit: b91b8a2

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@kaizen/components Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@Zystix Zystix changed the title Kzn 2877/scss to css Initial Conversion of SCSS to CSS Dec 18, 2024
@Zystix Zystix marked this pull request as ready for review December 19, 2024 21:56
@Zystix Zystix requested a review from a team as a code owner December 19, 2024 21:56
Copy link
Contributor

github-actions bot commented Dec 19, 2024

✨ Here is your branch preview! ✨

Last updated for commit b91b8a2: Simplification

Copy link
Contributor

@mcwinter07 mcwinter07 left a comment

Choose a reason for hiding this comment

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

I've left a couple of comment about some opportunities we have to clean up the variable usage in the CSS.

With the ButtonGroup I can see the a CSS module but a SCSS module deletion in the PR diff / an update to the CSS import.

packages/components/src/Avatar/Avatar.module.css Outdated Show resolved Hide resolved
packages/components/src/BrandMoment/BrandMoment.module.css Outdated Show resolved Hide resolved
packages/components/src/Badge/Badge.module.css Outdated Show resolved Hide resolved
packages/components/src/Badge/Badge.module.css Outdated Show resolved Hide resolved
packages/components/src/AvatarGroup/AvatarGroup.module.css Outdated Show resolved Hide resolved
@mcwinter07
Copy link
Contributor

Ooo, also I'd checkout the UI tests for the Avatar, a little bit of funkiness see to be happening in the Avatar stickersheets 😉

Copy link
Contributor

@HeartSquared HeartSquared left a comment

Choose a reason for hiding this comment

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

Let me know if you want to chat through any comments, or if you think any of it is too much work :) Most comments are suggestions for how to compose the CSS better (figured it's a good learning opportunity), but I'm open to descoping some of it if you have other priorities, since this is an improvement regardless.

packages/components/src/Avatar/Avatar.module.css Outdated Show resolved Hide resolved
packages/components/src/Avatar/Avatar.module.css Outdated Show resolved Hide resolved
packages/components/src/AvatarGroup/AvatarGroup.module.css Outdated Show resolved Hide resolved
packages/components/src/AvatarGroup/AvatarGroup.module.css Outdated Show resolved Hide resolved
packages/components/src/Badge/Badge.module.css Outdated Show resolved Hide resolved
packages/components/src/BrandMoment/BrandMoment.module.css Outdated Show resolved Hide resolved
packages/components/src/BrandMoment/BrandMoment.module.css Outdated Show resolved Hide resolved
packages/components/src/BrandMoment/BrandMoment.module.css Outdated Show resolved Hide resolved
packages/components/src/BrandMoment/BrandMoment.module.css Outdated Show resolved Hide resolved
packages/components/src/ButtonGroup/ButtonGroup.module.css Outdated Show resolved Hide resolved
border: 3px solid var(--color-white);
}

.wrapper.small {
Copy link
Contributor

Choose a reason for hiding this comment

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

Haha oh no! I must not have been clear enough. Here's an example:

Suggested change
.wrapper.small {
.small {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

😭

Comment on lines 16 to 54
.reversed {
--badge-background-color: rgba(var(--color-white-rgb), 0.1);

color: var(--color-white);
}

.reversed.active {
--badge-background-color: var(--color-green-300);

color: var(--color-purple-800);
}

.reversed.dark {
--badge-background-color: var(--color-purple-700);

color: var(--color-white);
}
}

.badge.large {
display: inline-flex;
justify-content: center;
border-radius: var(--spacing-48);
font-size: var(--typography-data-medium-font-size);
line-height: var(--typography-data-medium-line-height);
letter-spacing: var(--typography-data-medium-letter-spacing);
padding: 2px 1.875rem;
width: 24px;
}

.default {
color: var(--color-purple-800);
}

.active {
--badge-background-color: var(--color-blue-500);

color: var(--color-white);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully this helps a little in understanding the structure?

Suggested change
.reversed {
--badge-background-color: rgba(var(--color-white-rgb), 0.1);
color: var(--color-white);
}
.reversed.active {
--badge-background-color: var(--color-green-300);
color: var(--color-purple-800);
}
.reversed.dark {
--badge-background-color: var(--color-purple-700);
color: var(--color-white);
}
}
.badge.large {
display: inline-flex;
justify-content: center;
border-radius: var(--spacing-48);
font-size: var(--typography-data-medium-font-size);
line-height: var(--typography-data-medium-line-height);
letter-spacing: var(--typography-data-medium-letter-spacing);
padding: 2px 1.875rem;
width: 24px;
}
.default {
color: var(--color-purple-800);
}
.active {
--badge-background-color: var(--color-blue-500);
color: var(--color-white);
}
}
.reversed {
--badge-background-color: rgba(var(--color-white-rgb), 0.1);
color: var(--color-white);
}
// TODO: .dark.reversed moved into .dark
.large {
display: inline-flex;
justify-content: center;
border-radius: var(--spacing-48);
font-size: var(--typography-data-medium-font-size);
line-height: var(--typography-data-medium-line-height);
letter-spacing: var(--typography-data-medium-letter-spacing);
padding: 2px 1.875rem;
width: 24px;
// TODO:
// &.dot { ... } can probably go here, but it does mean that .dot must go above it.
// this .large can likely go further down in the file after the variants
}
.default {
color: var(--color-purple-800);
}
.active {
--badge-background-color: var(--color-blue-500);
color: var(--color-white);
&.reversed {
--badge-background-color: var(--color-green-300);
color: var(--color-purple-800);
}
}

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