Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a comprehensive CSS style guide and convention document for the KoliBri project, detailing component structure, theme layers, unit usage, and BEM naming patterns. The review feedback identifies several typos and grammatical errors, suggests clearer terminology for component relationships, and recommends adopting modern Sass best practices by using @use instead of @import.
|
|
||
| - every component has a style.scss with its basic definitions | ||
| - component styles have to be wrapped in `@layer kol-component` | ||
| - styles.scss is importet in its corresponding shadow.tsx |
There was a problem hiding this comment.
Typo: "importet" should be "imported".
| - styles.scss is importet in its corresponding shadow.tsx | |
| - styles.scss is imported in its corresponding shadow.tsx |
| - styles.scss is importet in its corresponding shadow.tsx | ||
| - components that are part of other components have their styles in the @shared folder | ||
| - here no layer is used, just plain `@mixin componentName` | ||
| - in this cases the component file (e.g. alert/style.scss) just includes the mixin (e.g. \_alert.scss) |
There was a problem hiding this comment.
Grammar: "in this cases" should be "in these cases".
| - in this cases the component file (e.g. alert/style.scss) just includes the mixin (e.g. \_alert.scss) | |
| - in these cases the component file (e.g. alert/style.scss) just includes the mixin (e.g. \_alert.scss) |
| - components that are part of other components have their styles in the @shared folder | ||
| - here no layer is used, just plain `@mixin componentName` | ||
| - in this cases the component file (e.g. alert/style.scss) just includes the mixin (e.g. \_alert.scss) | ||
| - every component has to `@include` the styles of every containing component (e.g. icon has to be included in buttons, paging, etc.) |
There was a problem hiding this comment.
Clarity: "containing component" usually refers to the parent component. In this context, it seems to mean the components used within (like icons in buttons). "Component it contains" or "contained component" would be clearer.
| - every component has to `@include` the styles of every containing component (e.g. icon has to be included in buttons, paging, etc.) | |
| - every component has to include the styles of every component it contains (e.g. icon has to be included in buttons, paging, etc.) |
| - all styles are imported in /src/components/index.ts | ||
| - components that are part of other components have their styles in the @shared folder | ||
| - here no layer is used, just plain `@mixin componentName` | ||
| - in this cases the component file (e.g. alert.scss) just includes the mixin (e.g. \_alert.scss) |
There was a problem hiding this comment.
Grammar: "in this cases" should be "in these cases".
| - in this cases the component file (e.g. alert.scss) just includes the mixin (e.g. \_alert.scss) | |
| - in these cases the component file (e.g. alert.scss) just includes the mixin (e.g. \_alert.scss) |
| - components that are part of other components have their styles in the @shared folder | ||
| - here no layer is used, just plain `@mixin componentName` | ||
| - in this cases the component file (e.g. alert.scss) just includes the mixin (e.g. \_alert.scss) | ||
| - every component has to @include the styles of every containing component (e.g. icon has to be included in buttons, paging, etc.) |
There was a problem hiding this comment.
Clarity: "containing component" should be "component it contains". Also, consider wrapping @include in backticks for consistency with the Components section.
| - every component has to @include the styles of every containing component (e.g. icon has to be included in buttons, paging, etc.) | |
| - every component has to include the styles of every component it contains (e.g. icon has to be included in buttons, paging, etc.) |
| When defining `rem` values within KoliBri, always use the provided Sass function: | ||
|
|
||
| ``` | ||
| @import '@shared/mixins'; |
There was a problem hiding this comment.
Modern Sass (Dart Sass) recommends using @use instead of @import for better module management and to avoid global namespace pollution. Since the project uses @use in other SCSS files, the documentation should reflect this best practice.
| @import '@shared/mixins'; | |
| @use '@shared/mixins' as *; |
| } | ||
| ``` | ||
|
|
||
| When you need to combine modifiers, please us CSS properties. |
There was a problem hiding this comment.
Typo: "us" should be "use".
| When you need to combine modifiers, please us CSS properties. | |
| When you need to combine modifiers, please use CSS properties. |
| ``` | ||
|
|
||
| When you need to combine modifiers, please us CSS properties. | ||
| In the following example, we wan't a different styling for the _Close-Button-Element_ when the component itself has the _card_-Modifier: |
There was a problem hiding this comment.
Typo: "wan't" should be "want".
| In the following example, we wan't a different styling for the _Close-Button-Element_ when the component itself has the _card_-Modifier: | |
| In the following example, we want a different styling for the _Close-Button-Element_ when the component itself has the _card_-Modifier: |
| } | ||
| ``` | ||
|
|
||
| Avoid - @root to combine selectors: |
|
Netlify Draft Deployment |
1 participant
No description provided.